150274 - User should be able to reorder the Newsgroups like in NS4

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Pascal Chevrel:pascalc (PTO until April 26)]

In NS4, you could reorder newsgroups by clicking and dragging them where you
wanted in the list. It was a very valuable option for those who participate to
many different newsgroups. Now, you can no longer do it.

Comment 1

[XandreX]

yes indeed, everytime I want to sort the newsgroups inside a newsgroup server I
have to close Mozilla, and go&tweak the configuration file. Particularly when I
subscribe to a new newsgroup.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Confirming.  Unsetting milestone, since that should be set by someone actually
working on the bug.  See also bug 97186.

Comment 3

[Daniel Wang]

tip on how to manually reorder ng available at bug 97186 :)

Comment 4

[Pascal Chevrel:pascalc (PTO until April 26)]

Bug still here with build 2003042208
- removing mozilla1.0.1 keyword
- Adding polish and nsbeta1 keyword, this is the kind of bugs that make Netscape
release look unpolished compared to NS4

Comment 5

[(not reading, please use [e-mail] instead)]

not a blocker.

Comment 6

[Samir Gehani]

adt: nsbeta1-

Comment 7

[Boris Folgmann]

Any progress here?

Comment 8

[Boris Folgmann]

Please compare bug 61078 for a similar request.

Comment 9

[Michael]

Here are some very related bugs...perhaps the people involved could consolidate
on this?

Bugzilla Bug 61078 - Ability to Change Order of Accounts
Bugzilla Bug 97186 - Method of having alphabetical sort for newsgroups in the
folder pane.
Bugzilla Bug 150274 - User should be able to reorder the Newsgroups like in NS4

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #9)
> Here are some very related bugs...perhaps the people involved could consolidate
> on this?
> 
> Bugzilla Bug 61078 - Ability to Change Order of Accounts
> Bugzilla Bug 97186 - Method of having alphabetical sort for newsgroups in the
> folder pane.
> Bugzilla Bug 150274 - User should be able to reorder the Newsgroups like in NS4

It is up to the UI owner (Neil for SeaMonkey and Scott for Thunderbird) and the developers who 
implement this feature, if at all, to decide the way in which it will be fixed.  Duping all of these bugs 
into one won't help.  They are very clearly asking for separate things.

Comment 11

[John David Galt]

I've found this workaround: exit Mozilla (or
Thunderbird), then text-edit the file
profile_directory/News/news_server_name.rc, which appears to be nothing more
than a trn-style .newsrc, and move the newsgroup lines to the order in which
you want them displayed.

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

Already mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=97186#c5

Comment 13

[a_geek]

Voting for this bug because it's a nuisance. Hand-editing the config file is "counterintuitive" and doesn't fit with the rest of the application. But thanks for the workaround anyway.

Comment 14

[a_geek]

Voting for this bug because it's a nuisance. Hand-editing the config file is "counterintuitive" and doesn't fit with the rest of the application. But thanks for the workaround anyway.

NB: Sorting doesn't work in TB, too.

Platform: Linux i386, TB 1.5

Comment 15

[John David Galt]

Agree, this bug should have been filed under product=Core, component=Mail/News Front End.

Comment 16

[Markus Hossner]

Attachment: Patch

Patch enables changing of the newsgroup order by drag and drop. Drop newsgroup1 on newsgroup2 results in the new newsgroup order: ... newsgroup1 newsgroup2 ...

Patch adds a new function moveBefore(aFolder1, aFolder2); to the nsIMsgNewsFolder interface and changes the drag and drop for newsgroups.

Patch works for Seamonkey and Thunderbird (both use the same files)

Comment 17

[David :Bienvenu]

Comment on attachment 271476 [details] [diff] [review]
Patch

this will make some folks very happy, thx for the patch!

No need for this:

+      else
+      {
+        // do nothing for currentFolderSupports == folderSupports2
+      }

In fact, if I'm reading the diff correctly,

+    else
+    {
+      if (currentFolderSupports != folderSupports1)

can be

else if(...)


again, your editor has changed this (in fact, your other patch failed to apply because of this change):

- *   Håkan Waara  <[e-mail]>
+ *   HÃ¥kan Waara  <[e-mail]>

I think (hope) that the prevailing braces style is

if (a)
{
}

not 

if (a) {
}

probably should add 9000 as a const, since it's now a magic number we use in two places,

e.g., const int kNewsSortOffset;

Other than those things, the patch looks good to me.

It would really be nice if it could be extended to add drag+drop re-ordering of accounts in the folder pane as well.

Comment 18

[David :Bienvenu]

oh, one other thing - nsINewsFolder::moveBefore(aFolder1, aFolder2)

One of the folders getting moved before is "this" folder, so there shouldn't need to be two args to this method...

Comment 19

[Markus Hossner]

Attachment: Patch 2

> No need for this:
...

Changed.


> - *   Håkan Waara  <[e-mail]>
> + *   HÃ¥kan Waara  <[e-mail]>

Changed.

> e.g., const int kNewsSortOffset;

Done.

> One of the folders getting moved before is "this" folder, so there shouldn't
> need to be two args to this method...

No, "this" folder is the root folder. nsINewsFolder::moveBefore(aFolder1, aFolder2) is called as function of the root folder to change the order of its subfolders.

Comment 20

[David :Bienvenu]

Comment on attachment 271581 [details] [diff] [review]
Patch 2

Ah, I see now. Ok, you're going to hate me :-) but I hate seeing methods on a folder that are really meant for the root folder. I'd prefer that the method be on the nsNNTPIncomingServer instead, since you're really re-arranging groups on the server object, not the root folder.

Comment 21

[Markus Hossner]

(In reply to comment #20)
> I'd prefer that the method be on the nsNNTPIncomingServer instead, since
> you're really re-arranging groups on the server object, not the root folder.

I understand what you mean but I guess there is no other way than doing it in nsNewsFolder.cpp. To change the order of the newsgroups (subfolders of the root folder) we need access to mSubfolders of the root folder. And so we need to be in nsNewsFolder. 

There are other "root" functions too in nsNewsFolder like LoadNewsrcFileAndCreateNewsgroups() or AddNewsgroup(...) which sets the inicial newsgroup order and therefore too needs access to mSubfolders.

(And at least the patch checks if "this" folder really is the root folder (rootFolder1 != this) -> ERROR)

Comment 22

[David :Bienvenu]

Comment on attachment 271581 [details] [diff] [review]
Patch 2

Thx for the patch, Markus - yes, you're right. You could work around it with a static_cast to nsNewsFolder of the nntp incoming server's rootFolder and some friend magic, but that's probably worse.

you missed a brace here:
+    if (currentFolderSupports == folderSupports2) {

Comment 23

[[e-mail]]

Comment on attachment 271581 [details] [diff] [review]
Patch 2

>+        folderTree.builder.rebuild();
This is wrong. Somewhere along the line, you should notify RDF properly, so that all the other trees, such as the location toolbar, or a second message window, will update correctly. You may find that the following notifications will work:
1. NotifyItemRemoved
2. (update the sort order)
3. NotifyItemAdded

Comment 24

[Markus Hossner]

Attachment: Patch 3

1. Moving algorithm changed to a more efficient one.

2. NotifyItemRemoved and NotifyItemAdded added for each element which gets changed.

Comment 25

[[e-mail]]

Comment on attachment 271854 [details] [diff] [review]
Patch 3

I'm giving you r+ because this works. But, you may want to fix these in this or in a followup bug.

Dropping on the group after the current group does nothing. It would be really neat if you could arrange to drop before/after rather than on the group. You would need to change moveBefore(source, target) to moveFolder(source, target, orientation). Failing that, groups should drop away, so that when you drop on a group after the source group it should drop after instead of before.

>+    trans.addDataFlavor("text/x-moz-newsfolder");
I don't know if this is a good idea. You can always look at the type of the source folder.

>-        } else if (dataFlavor.value == "text/x-moz-url") {
>+        }
I don't suppose you can fix the other } else too? It looks lonely ;-)

>+          var sourceFolder = sourceResource.QueryInterface(Components.interfaces.nsIMsgFolder);
Since you're using this twice now I'd move the declaration to just after sourceResource.

>+          // don't allow dragging news folder (newsgroup) to other account
>+          if (targetFolder.rootFolder.URI != sourceFolder.rootFolder.URI)
>+            return false;
I think you copied existing bad code, but you don't need to compare URIs, the rootFolder objects should be identical. I don't know whether it's best to compare the rootFolder or the server object.

>+          // don't allow dragging news folder (newsgroup) to server folder
>+          var isServer = GetFolderAttribute(folderTree, targetResource, "IsServer");
>+          if (isServer == "true")
>+            return false;
Again, you copied bad code, but you can use if (targetFolder.isServer) instead. You also copied some "blank" lines containing spaces. Really blank please ;-)

>+      if (GetFolderAttribute(folderTree, sourceResource, "ServerType") == "nntp")
In fact, you do look at the type of the folder here. Good! But I think sourceFolder.server.type also works.

>-            messenger.copyFolders(GetFolderDatasource(), targetResource, list, (sourceServer == targetServer));
>+          messenger.CopyFolders(GetFolderDatasource(), targetResource,
>+                                list, (sourceServer == targetServer));
I think this is unintentional?

>+NS_IMETHODIMP nsMsgNewsFolder::MoveBefore(nsIMsgFolder *aFolder1, nsIMsgFolder *aFolder2)
Bad names. aSource and aTarget perhaps? (Same goes for the .idl)

>+  // get root folders
>+  nsCOMPtr<nsIMsgFolder> rootFolder1;
>+  rv = aFolder1->GetRootFolder(getter_AddRefs(rootFolder1));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIMsgFolder> rootFolder2;
>+  rv = aFolder2->GetRootFolder(getter_AddRefs(rootFolder2));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // if root folders differ exit with error
>+  if (rootFolder1 != rootFolder2 || rootFolder1 != this)
>+    return NS_ERROR_FAILURE;
>+ 
>+  // get supports for folders
>+  nsCOMPtr<nsISupports> folderSupports1 = do_QueryInterface(aFolder1, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsISupports> folderSupports2 = do_QueryInterface(aFolder2, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // find folder1 and folder2 and change the sort order for all elements between them
OK, so the easy way to find folder1 and folder2 is to use mSubFolders->IndexOf, if either of them return -1 then you know the argument is invalid (return NS_ERROR_INVALID_ARG).

Your renumbering code looks overly complex. Surely once you've called MoveElement it merely remains to fix up the sort order values to be equal to the new index? Also, I don't like you returning while the notifications are in an inconsistent state.

>+  // write changes back to file
>+  rv = SetNewsrcHasChanged(PR_TRUE);
>+  NS_ENSURE_SUCCESS(rv,rv);
>+
>+  nsCOMPtr<nsINntpIncomingServer> nntpServer;
>+  rv = GetNntpServer(getter_AddRefs(nntpServer));
>+  NS_ENSURE_SUCCESS(rv,rv);
SetNewsrcHasChanged is a convenience method; since you're fetching the server anyway you might as well call it directly on the server.

One last thing I noticed was that the folder becomes deselected, but presumably that's just a case of calling SelectFolder after moving the folder.

Comment 26

[[e-mail]]

Attachment: Proof of concept

This contains some of the changes I mentioned above.

Comment 27

[Markus Hossner]

Attachment: Patch 4

(In reply to comment #25)
> Dropping on the group after the current group does nothing. It would be really
> neat if you could arrange to drop before/after rather than on the group.

Yes, this may be a followup bug.


> Failing that, groups should drop away, so that when you drop on a
> group after the source group it should drop after instead of before.

I prefer a consistent behaviour. Drop A on B results in ... A B ...


> >+          var sourceFolder = sourceResource.QueryInterface(Components.interfaces.nsIMsgFolder);
> Since you're using this twice now I'd move the declaration to just after
> sourceResource.

But sourceResource could be a nsIRDFResource too.


> >+NS_IMETHODIMP nsMsgNewsFolder::MoveBefore(nsIMsgFolder *aFolder1, nsIMsgFolder *aFolder2)
> Bad names. aSource and aTarget perhaps? (Same goes for the .idl)

Yes bad names. But which to choose. Folder1 is not really used as a source for anything.


> Your renumbering code looks overly complex.

As I said more efficient algorithm. Just one loop through the subfolders. But it is indeed more complex. ;-)


+  NotifyItemRemoved(aFolder1);
+  mSubFolders->MoveElement(indexFolder1, indexFolder2);
+
+  // we only need to do the range 1 to 2 but I'm lazy
+  for (PRUint32 i = 0; i < cnt; i++)
+  {
+    nsCOMPtr<nsIMsgFolder> currentFolder(do_QueryElementAt(mSubFolders, i));
+    if (currentFolder)
+      currentFolder->SetSortOrder(kNewsSortOffset + i);
+  }
+
+  NotifyItemAdded(aFolder1);

This code does not work properly. You have to inform the RDF about every change you make. Not informing it about the new sort codes of the folders inbetween will result in a wrong sort order, cause the front end does not change the sort codes of the other folders if there is no notification.


Thanks for all the other comments too. I applied them in the new patch.

Comment 28

[[e-mail]]

(In reply to comment #27)
>(In reply to comment #25)
>>Failing that, groups should drop away, so that when you drop on a
>>group after the source group it should drop after instead of before.
>I prefer a consistent behaviour. Drop A on B results in ... A B ...
But that does make it impossible to move a group to the end...

>>>+          var sourceFolder = sourceResource.QueryInterface(Components.interfaces.nsIMsgFolder);
>>Since you're using this twice now I'd move the declaration to just after
>>sourceResource.
>But sourceResource could be a nsIRDFResource too.
I'm not sure what you're saying here. But in case you misunderstood me, I mean add "var sourceFolder;" after "var sourceResource;" and drop the var here and the other place it is used.

>>>+NS_IMETHODIMP nsMsgNewsFolder::MoveBefore(nsIMsgFolder *aFolder1, nsIMsgFolder *aFolder2)
>>Bad names. aSource and aTarget perhaps? (Same goes for the .idl)
>Yes bad names. But which to choose. Folder1 is not really used as a source for
>anything.
Well, they're called source and target in messengerdnd.js ;-)

>This code does not work properly. You have to inform the RDF about every change
>you make. Not informing it about the new sort codes of the folders inbetween
>will result in a wrong sort order, cause the front end does not change the sort
>codes of the other folders if there is no notification.
Odd, I didn't think the front end saved the sort codes of the other folders.

Comment 29

[Markus Hossner]

Attachment: Patch 5

Here we go again. Thanks to Neil reorder newsgroups now works like reorder bookmarks in the bookmarks manager.

Comment 30

[[e-mail]]

Comment on attachment 272088 [details] [diff] [review]
Patch 5

So, if you move group 1 to before group 3, you reorder groups 2 and 3, and then you have to reorder group 3 again later. Very hacky :s

Comment 31

[[e-mail]]

Attachment: Inform the RDF

Based on my previous proof of concept patch, this shows how to inform the RDF on every sort order change so that you only have to notify the removal and add of the newsgroup actually being moved. Note: callers to NotifyIntPropertyChanged normally already have an atom somewhere but I didn't look to find out where.

Comment 32

[Markus Hossner]

Attachment: Patch 6

Now only with notifying the removal and add of the moved newsgroup.

Comment 33

[[e-mail]]

Comment on attachment 272318 [details] [diff] [review]
Patch 6

You seem to have spotted all the mistakes in my proof of concept :-)

Comment 34

[David :Bienvenu]

Comment on attachment 272318 [details] [diff] [review]
Patch 6

did a tab sneak in here?

+	       // don't allow dragging news folder (newsgroup) to before/after itself
+          index += orientation;


-            messenger.copyFolders(GetFolderDatasource(), targetResource, list, (sourceServer == targetServer));
+          messenger.CopyFolders(GetFolderDatasource(), targetResource, list, (sourceServer == targetServer));

that looks wrong - in nsIMessenger.idl, it's copyFolders, not CopyFolders.

sr=bienvenu, if you fix those two things.

Comment 35

[Markus Hossner]

Attachment: patch for checkin

fixing David's comment #27, taking over r+ and sr+ (I don't have the right to set it)

Comment 36

[Karsten Düsterloh]

Landed on trunk. Yay!

Comment 37

[Joe Sabash [:JoeS1]]

Testing in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071815 Thunderbird/3.0a1pre ID:2007071815
The drag seems to work fine, but the new position does not persist over sessions.

Comment 38

[Ruediger Lahl]

(In reply to comment #35)

> patch for checkin

Thanks Markus. One little bug:
choose a group and set an old message to unread. Then click to an other group. After that, grab the former group to move it to another place. In the moment you grab the group, the message you set on unread is marked as read again.

The bug only appears one time. If you mark more than on message to unread, only the first one is affected. If you move the group again, no more messages are set to read.

Tested on: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a7pre) Gecko/200707190606 SeaMonkey/2.0a1pre

Comment 39

[Magnus Melin [:mkmelin]]

I think its better if you can file new bugs for remaining issues, then mark them as blocking this one.

(The new position persists for me, on linux. Nor do I see the unread issue.)

Comment 40

[(not reading, please use [e-mail] instead)]

fyi, see the leak fix from boris in bug #405451