Closed Bug 205378 Opened 22 years ago Closed 22 years ago

Meta bug for changes in bookmarks sorting code

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.4final

People

(Reporter: janv, Assigned: janv)

References

Details

(Whiteboard: [a=sspitzer, a=asa])

Attachments

(1 file, 3 obsolete files)

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.
Depends on: 64272, 64768, 77411, 149690, 158521, 204022
Depends on: 204211
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.4final
Depends on: 202845
Depends on: 119905, 121261
Depends on: 201048
Depends on: 199367
Depends on: 201246
Depends on: 201120
Depends on: 200200
Depends on: 205645
Attached patch patch (obsolete) (deleted) — Splinter Review
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 on attachment 123309 [details] [diff] [review] patch requesting r= from jag Pierre, could you take a look too ?
Attachment #123309 - Flags: review?(jaggernaut)
there are test builds for Linux and MacOS X, if you want to try it, let me know
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 !
Depends on: 144238
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!
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 ?
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.
Attachment #123309 - Flags: review?(jaggernaut)
Attached patch new patch (obsolete) (deleted) — Splinter Review
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);
Attachment #123309 - Attachment is obsolete: true
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 :)
Attachment #123488 - Flags: superreview?(sspitzer)
Attachment #123488 - Flags: review?(jaggernaut)
Attachment #123488 - Flags: approval1.4?
+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.
Attachment #123488 - Flags: superreview?(sspitzer)
Attachment #123488 - Flags: review?(jaggernaut)
Attachment #123488 - Flags: approval1.4?
Attached patch final patch (obsolete) (deleted) — Splinter Review
- fixed onEndUpdateBatch method in nsCompositeDataSource, nsXULTemplateBuilder and nsBookmarksService - fixed EndUpdateBatch in nsTreeBodyFrame - added a comment for ElementArray class
Attachment #123488 - Attachment is obsolete: true
Attachment #123540 - Flags: review?(jaggernaut)
Comment on attachment 123540 [details] [diff] [review] final patch jag says r=jag
Attachment #123540 - Flags: superreview?(sspitzer)
Attachment #123540 - Flags: review?(jaggernaut)
Attachment #123540 - Flags: review+
Attachment #123540 - Flags: approval1.4?
Hardware: PC → All
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?
cc'ing people to let them know about these UI changes
Can you please summarize the UI changes in this patch? This will allow me to assess the impact on the help. Thanks.
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
>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
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.
cc'ing yxia and rcloseirl
Robin, Do you have an estimate on the word count of the new text?
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).
Estimated word count for new/changed help text in customize_help.html is 50 words.
reviewing now, sorry for the delay.
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(); +}
+cmd_bm_sortfolderbyname = Sort Folder by name If this is the name of a menu item, it should be "Sort Folder by Name"
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.
r=bobj for UI and help changes
Blocks: 200048
Attached patch patch v1.0 (deleted) — Splinter Review
hope this is really final
Attachment #123540 - Attachment is obsolete: true
Attachment #123540 - Flags: superreview?(sspitzer)
Attachment #123540 - Flags: approval1.4?
Attachment #123980 - Flags: superreview?(sspitzer)
Attachment #123980 - Flags: review+
Attachment #123980 - Flags: approval1.4?
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.
Attachment #123980 - Flags: superreview?(sspitzer) → superreview+
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.
Whiteboard: [a=sspitzer, but wait for a=asa]
Comment on attachment 123980 [details] [diff] [review] patch v1.0 a=asa (on behalf of drivers) for checkin to 1.4.
Attachment #123980 - Flags: approval1.4? → approval1.4+
updating summary to reflect a=sspitzer,a=asa
Whiteboard: [a=sspitzer, but wait for a=asa] → [a=sspitzer, a=asa]
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.
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.
People are reporting bug 204022 fixed today. It looks like this was checked in this morning. Resolve Fixed?
Yes, I believed this was checked in since other depend bugs listed have been fixed in today's trunk.
Checkin was completed earlier this morning. Marking verfied.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
okay, folder sorting is working, but terribly. It basically screws up your bookmarks if you use separators to organize your bookmarks. but verifying anyway...
Status: RESOLVED → VERIFIED
Nobody is perfect, maybe I should have wait and fix the separators too, but then it would not make 1.4 ...
a=adt Please check into the Mozilla 1.4 branch and add the keyword fixed1.4
This was fixed right before branching for 1.4, so it's already on the branch.
Blocks: 210418
No longer blocks: 210418
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
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: