205378 - Meta bug for changes in bookmarks sorting code

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect, P1)

Description

[Jan Varga [:janv]]

This should cover all work I've done to fix some long standing issues with
sorting in bookmarks, and some other problems as well.
I'll add dependencies later.

Comment 1

[Jan Varga [:janv]]

Attachment: patch

Comment 2

[Jan Varga [:janv]]

comments on the patch:
- renamed nsIRDFObserver::begin/EndUpdateBatch to onBegin/onEndUpdateBatch
  to match style in this interface
- added begin/endUpdateBatch to nsIRDFDataSource which is actually the API to do  
  batch updates
- made nsXULTemplateBuilder::onEndUpdateBatch to rebuild its content and fixed
  all consumers
- introduced sortLocked attribute to lock view sort in bookmarks
- added nsIXULBuilderListener to be able to save and restore selection after a 
  batch operation
- fixed RebuildAll() to not crash when the document is being torn down
- added beginUpdateBatch to nsITreeBoxObject to fix flickering
- added nsIPropagatableDataSource (implemented by bookmarks service and
  in-memory data source) to be able disable/enable notifications completely
- removed startBatchUpdate/endBatchUpdate from nsIBrowserHistory
  since there is nsIRDFDataSource:begin/endUpdateBatch now
- added sortFolder() method to nsIBookmarksService
- back ported cloneFolder() from Phoenix and cleaned it up a bit
- added BookmarkTransaction as a base class for all bookmarks transactions
  to reuse come code
- fixed batching in bookmarks (we batch updates only if there are more than 8 
  items to insert or remove)
- removed sort menu items from bookmark manager's view menu
- selection is now correctly restored after sorting
- removed pref based sort synchronization
- it's not possible to drag the root folder now
- added column header tooltips (e.g. Click to sort by name)
- added sort folder dialog
- folders are now sorted first

Comment 3

[Jan Varga [:janv]]

Comment on attachment 123309 [details] [diff] [review]
patch

requesting r= from jag
Pierre, could you take a look too ?

Comment 4

[Jan Varga [:janv]]

there are test builds for Linux and MacOS X, if you want to try it, let me know

Comment 5

[Chris Petersen]

Using the macho test build, I checked all of the depend bugs mentioned. All of
these issues appeared resolved with this build. No regressions were encountered
either :). Looks good, Jan !

Comment 6

[Pierre Chanial]

I pulled a seamonkey tree yesterday and build it.
There is still some work to do concerning the CloneResource I've written, so the
best would be to split it since your patch is already really big and unrelated
to the paste horkage. I will attach a patch today.
Why did you remove the "ID" arc adjustement in CloneResource? If you do so, you
should also remove all its occurences.

other than that your work seems fantastic but I haven't have the time to read it
carefully yet!

Comment 7

[Jan Varga [:janv]]

Pierre, thanks for looking at the patch.

>Why did you remove the "ID" arc adjustement in CloneResource? If you do so, you
>should also remove all its occurences.

Actually, how the "ID" is used, and what other occureces do you mean ?

Comment 8

[Pierre Chanial]

ok, so I fixed the remaining issues related to pasting in bug 201048.
Concerning the "ID" arc I have not a strong opinion. It doesn't seem to be used,
but I will need more time to clearly assess it. I was under the impression that
having an ID that doesn't match the resource would be confusing, but I simply
added a comment and opened bug 205857.

Concerning the way you prevent the root folder to be dragged, you shouldn't need
to introduce a containsRT field to the selection. the root folder is immutable
and you should reuse/fix existing code. If you're short in time you can hand it
to me, I'll fix it together with bug 200067.

Comment 9

[Jan Varga [:janv]]

Attachment: new patch

Jag and I went over the original patch and found a few things which I fixed in
this new patch:
- removed XXX comments about not checking return values of OnAssert, etc.  
- changed sort consts to ALL_CAPS
- sortOptions.accepted = false -> var sortOptions = { accepted : false }
- moved begin/EndUpdateBatch() directly to sortFolder() 	
- changed |RDFC : RDFC| to |RDFC : null|	       
- removed a blank line
- changed |event.ctrlKey| to |event.ctrlKey || event.metaKey| to support mac 
correctly
- changed |0 <= --index| to |--index >= 0|
- changed |j--| to |--j|
- renamed isBookmarkedInternal() to isBookmarkedResource()
- added a check for folder type in cloneResource()
- added some comments				      

I didn't change only one thing. That NS_CONST_CAST can't be removed.
It didn't compile when I removed it.

nsBookmarksService::Compare(const void* aElement1, const void* aElement2)
{
  ElementInfo* elementInfo1 = NS_STATIC_CAST(ElementInfo*, aElement1);	
  const ElementInfo* elementInfo1 = NS_STATIC_CAST(ElementInfo*, aElement1);

Comment 10

[Jan Varga [:janv]]

Comment on attachment 123488 [details] [diff] [review]
new patch

We spent over 3 hours by reviewing all these changes and according to Chris he
hasn't found any regressions nor I
Anyway, I'll cross fingers before landing :)

Comment 11

[jag (Peter Annema)]

+nsXULTemplateBuilder::OnEndUpdateBatch(nsIRDFDataSource* aDataSource)
 {
+    if (mUpdateBatchNest > 0) {
+        if (--mUpdateBatchNest == 0) {
+            Rebuild();
+        }
+    }

Unless OnEndUpdateBatch can validly get called more often than
OnBeginUpdateBatch, I think you can get rid of the outer check there.

Though looking at CompositeDataSourceImpl::OnEndUpdateBatch and other
implementations (though I realize they could've copied this pattern from
eachother), I'm starting to wonder now. I thought OnStartUpdateBatch and
OnEndUpdateBatch always had to come in pairs... Is there a known valid case
where we issue OnEndUpdateBatch without or before OnStartUpdateBatch? If we
don't know, should we replace these (sanity?) checks with assertions?

+CompositeDataSourceImpl::OnBeginUpdateBatch(nsIRDFDataSource* aDataSource)
 {
     PRInt32 nest = mUpdateBatchNest++;
     if (nest == 0) {

Since nest isn't used elsewhere in that function, you could just inline it.



Could you add a comment to ElementArray explaining that any call on it that
would result in an element being removed or replaced should make sure that the
element gets deleted? Either that, or implement these methods to make sure the
right thing happens.

Comment 12

[Jan Varga [:janv]]

Attachment: final patch

- fixed onEndUpdateBatch method in nsCompositeDataSource, nsXULTemplateBuilder
and nsBookmarksService
- fixed EndUpdateBatch in nsTreeBodyFrame
- added a comment for ElementArray class

Comment 13

[Jan Varga [:janv]]

Comment on attachment 123540 [details] [diff] [review]
final patch

jag says r=jag

Comment 14

[Asa Dotzler [:asa]]

These changes are large and we need to get them in soon if they're gonna make
the 1.4 train. Seth, can you sr this?

Comment 15

[Jan Varga [:janv]]

cc'ing people to let them know about these UI changes 

Comment 16

[robinf]

Can you please summarize the UI changes in this patch? This will allow me to
assess the impact on the help. Thanks.

Comment 17

[Jan Varga [:janv]]

UI changes:
- all sort related items have been removed from the View menu in bookmarks manager
- added a new item to the View menu (x Search bar)
- added two new items to the Edit menu in the context menu (Sort Folder..., Sort
Folder by name)
- added a new dialog for "Sort Folder..." to customize sort options
- added a new warning after clicking on a column header that the sorting is
undoable ("If you sort this list, you will not be able to Undo it. Are you sure
you want to sort the list?")
- added tooltips for column headers in bookmarks manager (Sort to click by name,
...)
- sort indicators (arrows) are not used anymore (they might return, we are not
sure yet, assume they will not be there for now)

these locale files are affected:
xpfe/components/bookmarks/resources/locale/en-US/bookmarks.dtd
xpfe/components/bookmarks/resources/locale/en-US/bookmarks.properties
xpfe/components/bookmarks/resources/locale/en-US/sortFolder.dtd

hope I covered everything

Comment 18

[Jan Varga [:janv]]

>added two new items to the Edit menu in the context menu
should read, added two new items to the Edit menu *and* the context menu

Comment 19

[robinf]

Affected help file: customize_help.html (The description of sorting bookmarks
will need to be updated).

Bobj - if you approve the UI changes, please also add a comment saying whether
or not you approve the help file updates. 

Comment 20

[bobj]

cc'ing yxia and rcloseirl

Comment 21

[bobj]

Robin,
Do you have an estimate on the word count of the new text?

Comment 22

[bobj]

This is kinda a bug UI change this late in the game, but probably manageable...

I want to talk to the folks in Europe working on the localizations and I want to
talk to Robin about the size of the help change.

I should have a verdict on the L10N impact by tomorrow morning (PDT).

Comment 23

[robinf]

Estimated word count for new/changed help text in customize_help.html is 50 words.

Comment 24

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

reviewing now, sorry for the delay.

Comment 25

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

Comment on attachment 123540 [details] [diff] [review]
final patch

1)

these should not be hard coded

+		   <menuitem value="Name" label="Name"/>
+		   <menuitem value="URL" label="Location"/>
+		   <menuitem value="ShortcutURL" label="Keyword"/>
+		   <menuitem value="Description" label="Description"/>
+		   <menuitem value="BookmarkAddDate" label="Added"/>
+		   <menuitem value="LastModifiedDate" label="Last Modified"/>
+		   <menuitem value="LastVisitDate" label="Last Visited"/>

2)

these should not be hard coded

+		   <menuitem value="ascending" label="A > Z"/>
+		   <menuitem value="descending" label="Z > A"/>

3)

do you have l10n approval for all the new strings?

if so, maybe you could land the strings first.

+cmd_bm_sortfolderbyname = Sort Folder by name
+cmd_bm_sortfolder = Sort Folder...

 cmd_bm_openinnewwindow = Open in New Window
 cmd_bm_openinnewtab = Open in New Tab 
@@ -90,3 +92,6 @@

 search_button_label = Find

+confirm_sorting_title = Confirm
+confirm_sorting_message = If you sort this list, you will not be able to Undo
it. Are you sure you want to sort the list?
+confirm_sorting_check_message = Don't ask me this again


+<!ENTITY window.title "Sort Folder">
+
+<!ENTITY sortOptions.label "Sort options">
+<!ENTITY description.label "&brandShortName; can sort individual folders. Use
these options to customize the sorting for this Folder.">
+<!ENTITY sortBy.label "Sort by:">
+<!ENTITY sortOrder.label "Sort order:">
+<!ENTITY sortFoldersFirst.label "Sort folders first">
+<!ENTITY sortRecursively.label "Sort recursively">

4)

can these QI's fail?

+NS_IMETHODIMP
+nsBookmarksService::GetPropagateChanges(PRBool* aPropagateChanges)
+{
+    nsCOMPtr<nsIRDFPropagatableDataSource> propagatable =
do_QueryInterface(mInner);
+    return propagatable->GetPropagateChanges(aPropagateChanges);
+}
+
+NS_IMETHODIMP
+nsBookmarksService::SetPropagateChanges(PRBool aPropagateChanges)
+{
+    nsCOMPtr<nsIRDFPropagatableDataSource> propagatable =
do_QueryInterface(mInner);
+    return propagatable->SetPropagateChanges(aPropagateChanges);
+}
+

5)

can this QI fail?

+  if (--mBatches == 0) {
+    nsCOMPtr<nsIRDFRemoteDataSource> remote = do_QueryInterface(mDataSource);
+    rv = remote->Flush();
+  }
+  return rv;
 }

6)

is the style this way, or return NS_OK?

+NS_IMETHODIMP
+nsGlobalHistory::Init(const char* aURI)
+{
+	return(NS_OK);
+}
+
+
+
+NS_IMETHODIMP
+nsGlobalHistory::Refresh(PRBool aBlocking)
+{
+	return(NS_OK);

7)

a bunch of places like these, should we double check mInner?

+NS_IMETHODIMP
+RelatedLinksHandlerImpl::BeginUpdateBatch()
+{
+	 return mInner->BeginUpdateBatch();
+}
+
+
+
+NS_IMETHODIMP
+RelatedLinksHandlerImpl::EndUpdateBatch()
+{
+	 return mInner->EndUpdateBatch();
+}

Comment 26

[robinf]

+cmd_bm_sortfolderbyname = Sort Folder by name

If this is the name of a menu item, it should be "Sort Folder by Name"

Comment 27

[Jan Varga [:janv]]

1)
2)

>these should not be hard coded
Sorry, I must have overlooked it.
I moved all those menuitem labels to the DTD file.


3)

>do you have l10n approval for all the new strings?
Well, I cc'ed all people that should be aware of these changes, robinf already
commented in this bug.
I changed "Sort Folder by name" to "Sort Folder by Name" per his comment.

4)

>can these QI's fail?
>
>+NS_IMETHODIMP
>+nsBookmarksService::GetPropagateChanges(PRBool* aPropagateChanges)
>+{
>+    nsCOMPtr<nsIRDFPropagatableDataSource> propagatable =
>do_QueryInterface(mInner);
>+    return propagatable->GetPropagateChanges(aPropagateChanges);
>+}
>+

This can't fail until someone remove nsIPropagatableDataSource implementation
from nsInMemoryDataSource or someone change mInner to some other datasource.
I think it's safe to assume that this interface is always supported.

5)

>can this QI fail?

>+  if (--mBatches == 0) {
>+    nsCOMPtr<nsIRDFRemoteDataSource> remote = do_QueryInterface(mDataSource);
>+    rv = remote->Flush();
>+  }
>+  return rv;
> }

The same as above, mDataSource is an nsRDFXMLDataSource and that data source is
supposed to QI to nsIRDFRemoteDataSource.

6)

>is the style this way, or return NS_OK?
>
>+NS_IMETHODIMP
>+nsGlobalHistory::Init(const char* aURI)
>+{
>+	return(NS_OK);
>+}
>+
>+
>+
>+NS_IMETHODIMP
>+nsGlobalHistory::Refresh(PRBool aBlocking)
>+{
>+	return(NS_OK);

Actually, this code isn't new, I just removed startBatchUpdate() and
endBatchUpdate() because I added similar methods to nsIRDFDataSource interface
which this class implements. cvs diff is probably not smart enough to produce
more readable output.
The style in this file is not consistent, I can fix it if you want.

7)

>a bunch of places like these, should we double check mInner?
>
>+NS_IMETHODIMP
>+RelatedLinksHandlerImpl::BeginUpdateBatch()
>+{
>+	 return mInner->BeginUpdateBatch();
>+}
>+

The same as in 4)
Actually this is the style in this file. mInner is initialized in Init() method
of this class. If that fails an instance of this class shouldn't be used.

Comment 28

[bobj]

r=bobj for UI and help changes

Comment 29

[Jan Varga [:janv]]

Attachment: patch v1.0

hope this is really final

Comment 30

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

Comment on attachment 123980 [details] [diff] [review]
patch v1.0

sr=sspitzer

make sure you found all the js / C++ implementations of nsIRDFDatasource in
both the moz and ns trees.

Comment 31

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

noting a=sspitzer, but wait for a=asa (or another driver) before landing

reasons for approval

1)  QA has tested and approved a preliminary build
2)  we have l10n approval
3)  this blocks several nsbeta1+ / 1.4 blockers
4)  we're still before 1.4 RC1, and if we are going to take this, it should
happen before RC1.

Comment 32

[Asa Dotzler [:asa]]

Comment on attachment 123980 [details] [diff] [review]
patch v1.0

a=asa (on behalf of drivers) for checkin to 1.4.

Comment 33

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

updating summary to reflect a=sspitzer,a=asa

Comment 34

[Axel Hecht]

Jan, could you please clear up on the impact of the changes to RDF?
CCing Heikki, as we have a security review of RDF still going on.

Comment 35

[Jan Varga [:janv]]

The original batching schema was not very well designed IMHO.
Because of poor design some objects added own methods to start/end batching
(e.g. nsIBrowserHistory).
Consumers (history, bookmarks, download manager) had to call tree builder
observer directly. This it not correct, because there might be other
windows/panels open with the same data source.
So I added these two new methods to the nsIRDFDataSource interface and renamed
nsIRDFObserver::BeginUpdateBatch to OnBeginUpdateBatch (actually this is the
style in that interface).
Additionally, I've made the template builder to rebuild its content when a batch
operation is completed (OnEndUpdateBatch). This was needed mainly for bug 204022.

I asked myself several times if I want to do such a big change, but I've decided
rather to clean things up than add another hacks.

I didn't attend any meetings regarding security of the RDF module, but I think
that it just make sense to have these methods along with Assert, Unassert, etc.
Well, there might be an issue: if someone forget to call EndUpdateBatch(),
viewers won't be updated anymore.

Comment 36

[Greg K.]

People are reporting bug 204022 fixed today. It looks like this was checked in
this morning. Resolve Fixed?

Comment 37

[Chris Petersen]

Yes, I believed this was checked in since other depend bugs listed have been
fixed in today's trunk.

Comment 38

[Chris Petersen]

Checkin was completed earlier this morning. Marking verfied.

Comment 39

[Daniel Wang]

okay, folder sorting is working, but terribly. It basically screws up your
bookmarks if you use separators to organize your bookmarks.

but verifying anyway...

Comment 40

[Jan Varga [:janv]]

Nobody is perfect, maybe I should have wait and fix the separators too, but then
it would not make 1.4 ...

Comment 41

[Paul Wyskoczka]

a=adt Please check into the Mozilla 1.4 branch and add the keyword fixed1.4

Comment 42

[Jan Varga [:janv]]

This was fixed right before branching for 1.4, so it's already on the branch.

Comment 43

[u119030]

I deliberately arrange my bookmarks so I can use them in 
a structured way. I am not interested in sorting bookmarks 
whatsoever and any sorting of bookmark data would spoil all 
my arrangements.
Therefore, I think the request of bug # 64272 was a really bad idea, 
and the changes of # 205378, even though there is a warning dialog, 
may be harmful if someone - not being completely aware of the 
consequences or in a hurry - just applies a sort and thus looses much 
work.
Please reconsider this issue and if possible refrain from any 
sorting of bookmark data. If you don't, please add a configuration 
option to suppress this ill "feature".

-- When I looked at this comment form a few minutes ago, there was an 
option "REOPEN BUG" - now there is only "Leave as VERIFIED FIXED" with 
no choice. Why did that change? Of course my comment means I suggest 
to re-open the bug.l