340324 - There is no Local Directory field in settings for RSS account

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Arkady Belousov]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows 98; ru) Opera 8.54
Build Identifier: http://mozilla-chi.osuosl.org/pub/mozilla.org/thunderbird/releases/1.5.0.4/win32/ru/Thunderbird%20Setup%201.5.0.4.exe

TB creates directory for RSS acount very deeply (<default>\Mail\News & Blogs) and there is no options to place content in other (custom) directories. The more so, I see there no relations with name of account, which I create.

BTW, there is something wrong with directories for News accounts: if I move directory for News account into other place, then TB beside directory also creates .msf file with same name in PARENT directory (ie, <dir>\<my news account>\ and <dir>\<my news account>.msf).

Reproducible: Always

Comment 1

[Mike Cowperthwaite]

*** This bug has been marked as a duplicate of 320571 ***

Comment 2

[Arkady Belousov]

> *** This bug has been marked as a duplicate of 320571 ***

     Sorry, absolutely no relations: I say about missing field in Preferences, whereas bug 320571 discusses nested attachments in forwarded mails.

Comment 3

[Mike Cowperthwaite]

You're right, that was the wrong bug.  I meant to dupe it to bug 307392.
However, I'm now thinking it's not strictly a dupe, only because the "Local Directory" setting doesn't appear in the "Disk Space" tab for other accounts.

Incidentally: you can make these changes yourself:
1) close TB, locate the prefs.js file in your profile, and edit it, OR
2) in the Config Editor, use a filter of ".directory"

Find the two references to the current path (.directory and .directory-rel) and replace them with the desired path.

Then either rename the existing directory, or create the new directory where you want it and move all the files from the old directory to the new one; and restart TB.

I don't recommend that you move the directory to somewhere outside of the profile directory, as the .directory-rel preference is a relative path starting at the profile directory.

Comment 4

[Arkady Belousov]

> Incidentally: you can make these changes yourself:
> 1) close TB, locate the prefs.js file in your profile, and edit it, OR

     As I understand, this is wrong way, because prefs.js is overwritten by TB when and as it wish. user.js also doesn't help much, because sometime TB forgets options from there (see my bug 339600 comment #5 - BTW, TB forgets edited options after opening any separate window).

> 2) in the Config Editor, use a filter of ".directory"

     Same trouble - see my bug 339600.

> I don't recommend that you move the directory to somewhere outside of the
> profile directory, as the .directory-rel preference is a relative path starting
> at the profile directory.

     _All_ of my custom directories I place much higher. For example, TB places its profile into %appdata%\Thunderbird and places profile very, very deeply in there. There I replace content of profiles.ini by "IsRelative=0/Path=%appdata%\TB\DEFAULT". Then I place all other directories in %appdata%\TB (for example, %appdata%\TB\Local).

     BTW, Firefox scheme is worser - it places profiles.ini into %appdata%\Mozilla\Firefox.

Comment 5

[Mike Cowperthwaite]

(In reply to comment #4)
> > Incidentally: you can make these changes yourself:
> > 1) close TB, locate the prefs.js file in your profile, and edit it, OR
> 
> As I understand, this is wrong way, because prefs.js is overwritten by TB
> when and as it wish.

It's not as random as that.  Some settings might get changed, but the paths are not likely to be.  Possibly, since you have this other problem, you might see such a change.

OT: It seems to me there's a good chance that your moved-around directories could be the *cause* of the problem in bug 339600.  If you haven't properly gotten every setting to point to the correct directories, there could be interactions that make no sense.  I suggest creating a new profile, and let the directories exist where TB puts them -- see if you can then reproduce the kinds of problems you're seeing with settings getting overwritten.

Comment 6

[Arkady Belousov]

> OT: It seems to me there's a good chance that your moved-around directories
> could be the *cause* of the problem in bug 339600.  If you haven't properly
> gotten every setting to point to the correct directories, there could be
> interactions that make no sense.

     All, what I do, I do from TB itself (except copying contents of directory to new place after changing Local Directory field).

> I suggest creating a new profile, and let the
> directories exist where TB puts them

     How do this? Will this harm my existing profile?

> -- see if you can then reproduce the kinds
> of problems you're seeing with settings getting overwritten.

Comment 7

[Wayne Mery (:wsmwk)]

(In reply to comment #6)
> > I suggest creating a new profile, and let the
> > directories exist where TB puts them
> 
>      How do this? Will this harm my existing profile?

create a new profile using profile manager. 
start from command line/dos window in thunderbird directory and use 
  thunderbird.exe -P

Comment 8

[:aceman]

I can try to expose the Local Directory in the account manager for Feeds. As the backend seems to handle it if done manually.

Comment 9

[:aceman]

Attachment: expose field

I think the Manage subscriptions button is now too close to the Browse button. It is a bit odd. Should I move it somewhere else?

Comment 10

[Ian Neal]

Comment on attachment 591898 [details] [diff] [review]
expose field

>+++ b/mailnews/extensions/newsblog/content/am-newsblog.xul
>@@ -49,18 +49,21 @@
>       onload="parent.onPanelLoaded('am-newsblog.xul');">
> 
>   <script type="application/javascript"
>           src="chrome://messenger/content/AccountManager.js"/>
>   <script type="application/javascript"
>           src="chrome://messenger-newsblog/content/am-newsblog.js"/>
>   <script type="application/javascript"
>           src="chrome://messenger-newsblog/content/newsblogOverlay.js"/>
>+  <script type="application/javascript"
>+          src="chrome://messenger/content/amUtils.js"/>
> 
>   <dialogheader id="am-newsblog-title" defaultTitle="&accountTitle.label;"/>
>+  <stringbundle id="bundle_prefs" src="chrome://messenger/locale/prefs.properties"/>
Presumably not needed with the current fix in bug 218439?

I agree, it does look a bit odd. I think there is probably a few options (not all mutually exclusive):
1/ Put button into a group box entitled "Subscriptions".
2/ Move button to top of page.
3/ Move the local directory to the disk space/storage page (this would change it for all account types though).
4/ Remove the button, news accounts don't have such a button.
There are probably other options too.

I do like the idea of doing at least 3 but that definitely needs a ui-review.

I have also noticed that the checkboxes should probably be in a group box entitled "Server Settings" to match other account types.

For the moment I'll leave the review request pending and see what Blake says wrt UI.

Comment 11

[:aceman]

Thanks. UI review is already requested. I'll remove the stringbundle line after that.

Comment 12

[:aceman]

I added the 'Server Settings' group header.
I'd like to add a "Subscribe button" to News but I find it hard to hook it to the Subscribe functions in the main Mail window.

Comment 13

[Ian Neal]

(In reply to :aceman from comment #12)
> I added the 'Server Settings' group header.
> I'd like to add a "Subscribe button" to News but I find it hard to hook it
> to the Subscribe functions in the main Mail window.

If you follow it through, you need to call the Subscribe function in mailCommands.js with the correct argument. Just a matter of making sure that mailCommands.js is within the scope of the account manager page.

Comment 14

[:aceman]

Yes, and that is what I have problem with. I am not yet able to follow the scope of things. If that function is somehow accessible with some window.top.x....Subscribe(). What tool can I use for that? DOM Inspector?

Comment 15

[Ian Neal]

(In reply to :aceman from comment #14)
> Yes, and that is what I have problem with. I am not yet able to follow the
> scope of things. If that function is somehow accessible with some
> window.top.x....Subscribe(). What tool can I use for that? DOM Inspector?

Not sure if there is a tool that does or not.
am-newsblog.xul loads newsblogOverlay.js, so perhaps am-server.xul could load mailCommands.js or the relevant functions moved to a file that is already loaded by am-server.xul or AccountManager.xul. Either way it is outside the scope of this bug.

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 591898 [details] [diff] [review]
expose field

So, I _really_ don't think people use that field, but it's on all the other account settings, so I guess for consistency we should add it here, too.  (Or perhaps we should remove it from all the other ones…  ;)

ui-r=me.

Thanks,
Blake.

Comment 17

[:aceman]

Bwinton, what about the Manage subscriptions button ?

Comment 18

[Blake Winton (:bwinton) (:☕️)]

(In reply to :aceman from comment #17)
> Bwinton, what about the Manage subscriptions button ?

Oh, yeah.  I didn't really notice it as being that out of place, perhaps because the rest of the settings aren't grouped.  (If you want to fix that, feel free, but I won't let it block the ui-r+ if you don't want to.)

Moving the Local Directory settings to the Disk Space page seems like a pretty big change, and I don't think we want to go that far just yet.

> 1/ Put button into a group box entitled "Subscriptions".

Since that would be the only group box, that seems strange.

> 2/ Move button to top of page.

Enh, all the Manage Identities buttons are at the bottom.

> 4/ Remove the button, news accounts don't have such a button.

I bet people would complain about this, too.

What about putting the Local Directory in a group, a la Local Folders: http://dl.dropbox.com/u/2301433/Screenshots/LocalFolders.png
?

At the end, I'ld be happy enough to leave it the way it is, so I'm going to let the ui-r+ stand, but if you wanted to add that group, that would be fine, too.

Thanks,
Blake.

Comment 19

[:aceman]

Thanks.
I have already grouped the other checkboxes to be under "Server settings" as other account types have it.

I can group the Local Directory even though it would be the only element in that group (on POP3 Server settings it does not have this grouping). But it would push the button further apart.

I'll try it.

Comment 20

[:aceman]

Attachment: patch, add option groupings

Adds the Server settings group into RSS panel. Also adds Message Storage group into RSS account pane, but also all Server settings panes. It looks quite nice and consistent. It also nicely contains the newsrc path and "empty thrash on exit" checkboxes on all account types. Please check all account types. The only problem is the Advanced button on IMAP account, which does not contain message storage options. The one on POP3 can be considered as such.

Comment 21

[Ian Neal]

(In reply to :aceman from comment #20)
> Created attachment 598290 [details] [diff] [review]
> patch, add option groupings
> 
> Adds the Server settings group into RSS panel. Also adds Message Storage
> group into RSS account pane, but also all Server settings panes. It looks
> quite nice and consistent. It also nicely contains the newsrc path and
> "empty thrash on exit" checkboxes on all account types. Please check all
> account types. The only problem is the Advanced button on IMAP account,
> which does not contain message storage options. The one on POP3 can be
> considered as such.

Trouble is IMAP and NNTP accounts have a pref panel called "Synchronization & Storage" which is confusing when you have group boxes called "Message Storage"

Comment 22

[:aceman]

That is true. However, that panel has options about which messages to store/delete. The Server settings panel/Local Directory and co. define where they are stored on disk. They are complementary. Maybe it just needs to reword the labels better.
Yes, it is possible to add the Message Storage group only to the RSS panel as bwinton suggested. I just found it inconsistent with the other panels, where the same field is not in any group (expect maybe Local Folders).

Comment 23

[Ian Neal]

For the moment, I would just do the Message Storage group for the RSS panel.
Potentially then a spin off bug for dealing with inconsistencies / code duplication / etc.

Comment 24

[Ian Neal]

Comment on attachment 598290 [details] [diff] [review]
patch, add option groupings

r- for the moment, missing SeaMonkey locale change

Comment 25

[:aceman]

Let's see what bwinton says before I delete half of the patch :)

Comment 26

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 598290 [details] [diff] [review]
patch, add option groupings

Yeah, I'm pretty happy with how this patch looks.  The only thing I might suggest is to add a sentence to the top saying "The following is a special account.  There are no identities associated with it", like the one in Local Folders.  But if you don't want to do that, that's okay, too.  :)

ui-r=me.

Thanks,
Blake.

Comment 27

[:aceman]

And what about Ian's comment 21? Have you seen all the panels in all account types?

Comment 28

[Blake Winton (:bwinton) (:☕️)]

I haven't, since I don't have a POP3 account, but I'm fine with having different settings for different types of account, and the blogs feel more to me like local folders than like IMAP or POP3 accounts.  (Also, we already have a group box called "Message Storage" in the local folders, so it's not like this patch is creating a new problem.)

For a longer-term fix, and this might be too big for you to want to tackle, I think we should merge the preferences and account settings, and have them be searchable, so that people will always be able to find the settings they're interested in, no matter where they are.  Something like MacOS X's Preferences, or Windows 7's Control Panel.

Comment 29

[:aceman]

Attachment: patch v2

I added the string as bwinton wished. I noticed most of the strings in am-serverwithnoidentities.dtd are duplicated in am-server-top.dtd. So I merged them a bit. I don't know if it is OK. The strings and accesskeys are identical in english. Are they identical in other languages? Is this optimization OK?

Comment 30

[Ian Neal]

Comment on attachment 599783 [details] [diff] [review]
patch v2

>+++ b/mailnews/base/prefs/content/am-server.xul

>+      <hbox align="end">
>+        <vbox align="start" flex="1" id="exitHandlingBox=">
>+          <checkbox hidefor="pop3,nntp,movemail" wsm_persist="true" id="imap.cleanupInboxOnExit"
>+                    label="&expungeOnExit.label;"
>+                    accesskey="&expungeOnExit.accesskey;"
>+                    prefattribute="value"
>+                    prefstring="mail.server.%serverkey%.cleanup_inbox_on_exit"/>
Nit: where you are touching code that has an element which is over multiple lines, could you have one attribute per line.
>+          <checkbox hidefor="nntp" wsm_persist="true" id="server.emptyTrashOnExit"
>+                    label="&emptyTrashOnExit.label;"
>+                    accesskey="&emptyTrashOnExit.accesskey;"
>+                    prefattribute="value"
>+                    prefstring="mail.server.%serverkey%.empty_trash_on_exit"/>
Nit: same again here.
>+        </vbox>
>+        <button label="&advancedButton.label;"
>+                accesskey="&advancedButton.accesskey;"
>+                oncommand="onAdvanced();"
>+                wsm_persist="true" id="server.advancedbutton"
>+                prefstring="mail.server.%serverkey%.advanced.disable"
>+                hidefor="nntp,movemail"/>
Nit: and here.
>+      </hbox>
>+
>+      <vbox hidefor="imap,pop3,movemail">
>+        <label value="&newsrcFilePath.label;" control="nntp.newsrcFilePath"/>
>+        <hbox align="center">
>+          <textbox readonly="true" wsm_persist="true" flex="1" id="nntp.newsrcFilePath"
>+                   datatype="nsILocalFile"
>+                   prefstring="mail.server.%serverkey%.newsrc.file" class="uri-element"/>
Nit: etc
>+          <button id="browseForNewsrc" label="&browseNewsrc.label;" filepickertitle="&newsrcPicker.label;"
>+                  accesskey="&browseNewsrc.accesskey;" oncommand="BrowseForNewsrc()"/>
Nit: and here, also have ; after the call in oncommand.
>+        </hbox>
>       </vbox>
>+      <separator class="thin"/>
>+
>+      <vbox>
>+      <label value="&localPath.label;" control="server.localPath"/>
>+        <hbox align="center">
>+          <textbox readonly="true" wsm_persist="true" flex="1" id="server.localPath" datatype="nsILocalFile"
>+                   prefstring="mail.server.%serverkey%.directory" class="uri-element"/>
Nit: and here
>+          <button id="browseForLocalFolder" label="&browseFolder.label;" filepickertitle="&localFolderPicker.label;"
>+                  accesskey="&browseFolder.accesskey;" oncommand="BrowseForLocalFolders()"/>
Nit: and here, also have ; after the call in oncommand.
>+        </hbox>
>+      </vbox>
>+
>     </groupbox>
> 
>     <separator class="thin"/>
>+
>+    <hbox hidefor="imap,pop3,movemail" align="center" valign="middle" iscontrolcontainer="true">
>+      <label value="&serverDefaultCharset.label;" control="nntp.charset"/>
>+      <menulist hidable="true" hidefor="imap,pop3,movemail" wsm_persist="true" id="nntp.charset"
>+                ref="NC:DecodersRoot" datasources="rdf:charset-menu"
>+                preftype="string" prefstring="mail.server.%serverkey%.charset">
Nit: and here

>+++ b/mailnews/base/prefs/content/am-serverwithnoidentities.xul
>+<!DOCTYPE page [
>+<!ENTITY % accountNoIdentDTD SYSTEM "chrome://messenger/locale/am-serverwithnoidentities.dtd" >%accountNoIdentDTD;
>+<!ENTITY % trashDTD SYSTEM "chrome://messenger/locale/am-server-top.dtd">%trashDTD;
Nit: need a better name for the entity, accountServerTopDTD perhaps?

>+++ b/mailnews/extensions/newsblog/content/am-newsblog.xul
>@@ -36,86 +36,114 @@
> 
> <?xml-stylesheet href="chrome://messenger/skin/accountManage.css" type="text/css"?>
> 
> <!DOCTYPE page [
> <!ENTITY % newsblogDTD SYSTEM "chrome://messenger-newsblog/locale/am-newsblog.dtd" >
> %newsblogDTD;
> <!ENTITY % accountNoIdentDTD SYSTEM "chrome://messenger/locale/am-serverwithnoidentities.dtd" >
> %accountNoIdentDTD;
>+<!ENTITY % trashDTD SYSTEM "chrome://messenger/locale/am-server-top.dtd">%trashDTD;
Nit: need a better name for the entity, accountServerTopDTD perhaps?

>+      <vbox>
>+        <label value="&localPath.label;" control="server.localPath"/>
>+        <hbox align="center">
>+          <textbox readonly="true" wsm_persist="true" flex="1" id="server.localPath" datatype="nsILocalFile"
>+                   prefstring="mail.server.%serverkey%.directory" class="uri-element"/>
Nit: where you are touching code that has an element which is over multiple lines, could you have one attribute per line.

>+          <button id="browseForLocalFolder" label="&browseFolder.label;" filepickertitle="&localFolderPicker.label;"
>+                  accesskey="&browseFolder.accesskey;" oncommand="BrowseForLocalFolders()"/>
Nit: and here, also have ; after the call in oncommand.

r- due to the number of nits.

Comment 31

[:aceman]

Attachment: patch v3

Thanks, fixed.

Comment 32

[Mark Banner (:standard8)]

Comment on attachment 601624 [details] [diff] [review]
patch v3

Review of attachment 601624 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/locales/en-US/chrome/messenger/am-server-top.dtd
@@ +78,5 @@
>  <!ENTITY browseNewsrc.label "Browse…">
>  <!ENTITY browseNewsrc.accesskey "e">
> +
> +<!ENTITY accountTitle.label "Account Settings">
> +<!ENTITY accountSettingsDesc.label "The following is a special account.  There are no identities associated with it.">

nit: just use one space please. The backend hides the double space anyway, and I believe the UI generally only uses one space.

Comment 33

[:aceman]

Attachment: patch v4

Thanks, space fixed (it was there before, I just moved it). r=IanN,standard8.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/085ffc3c8da5