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)
SeaMonkey
Bookmarks & History
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)
(deleted),
patch
|
janv
:
review+
sspitzer
:
superreview+
asa
:
approval1.4+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•22 years ago
|
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.4final
Assignee | ||
Updated•22 years ago
|
Assignee | ||
Comment 1•22 years ago
|
||
Assignee | ||
Comment 2•22 years ago
|
||
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
Assignee | ||
Comment 3•22 years ago
|
||
Comment on attachment 123309 [details] [diff] [review]
patch
requesting r= from jag
Pierre, could you take a look too ?
Attachment #123309 -
Flags: review?(jaggernaut)
Assignee | ||
Comment 4•22 years ago
|
||
there are test builds for Linux and MacOS X, if you want to try it, let me know
Comment 5•22 years ago
|
||
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•22 years ago
|
||
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!
Assignee | ||
Comment 7•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Updated•22 years ago
|
Attachment #123309 -
Flags: review?(jaggernaut)
Assignee | ||
Comment 9•22 years ago
|
||
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);
Assignee | ||
Updated•22 years ago
|
Attachment #123309 -
Attachment is obsolete: true
Assignee | ||
Comment 10•22 years ago
|
||
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?
Comment 11•22 years ago
|
||
+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.
Assignee | ||
Updated•22 years ago
|
Attachment #123488 -
Flags: superreview?(sspitzer)
Attachment #123488 -
Flags: review?(jaggernaut)
Attachment #123488 -
Flags: approval1.4?
Assignee | ||
Comment 12•22 years ago
|
||
- fixed onEndUpdateBatch method in nsCompositeDataSource, nsXULTemplateBuilder
and nsBookmarksService
- fixed EndUpdateBatch in nsTreeBodyFrame
- added a comment for ElementArray class
Attachment #123488 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #123540 -
Flags: review?(jaggernaut)
Assignee | ||
Comment 13•22 years ago
|
||
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?
Comment 14•22 years ago
|
||
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?
Assignee | ||
Comment 15•22 years ago
|
||
cc'ing people to let them know about these UI changes
Comment 16•22 years ago
|
||
Can you please summarize the UI changes in this patch? This will allow me to
assess the impact on the help. Thanks.
Assignee | ||
Comment 17•22 years ago
|
||
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
Assignee | ||
Comment 18•22 years ago
|
||
>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•22 years ago
|
||
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•22 years ago
|
||
cc'ing yxia and rcloseirl
Comment 21•22 years ago
|
||
Robin,
Do you have an estimate on the word count of the new text?
Comment 22•22 years ago
|
||
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•22 years ago
|
||
Estimated word count for new/changed help text in customize_help.html is 50 words.
Comment 24•22 years ago
|
||
reviewing now, sorry for the delay.
Comment 25•22 years ago
|
||
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•22 years ago
|
||
+cmd_bm_sortfolderbyname = Sort Folder by name
If this is the name of a menu item, it should be "Sort Folder by Name"
Assignee | ||
Comment 27•22 years ago
|
||
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•22 years ago
|
||
r=bobj for UI and help changes
Assignee | ||
Comment 29•22 years ago
|
||
hope this is really final
Attachment #123540 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #123540 -
Flags: superreview?(sspitzer)
Attachment #123540 -
Flags: approval1.4?
Assignee | ||
Updated•22 years ago
|
Attachment #123980 -
Flags: superreview?(sspitzer)
Attachment #123980 -
Flags: review+
Attachment #123980 -
Flags: approval1.4?
Comment 30•22 years ago
|
||
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+
Comment 31•22 years ago
|
||
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 32•22 years ago
|
||
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+
Comment 33•22 years ago
|
||
updating summary to reflect a=sspitzer,a=asa
Whiteboard: [a=sspitzer, but wait for a=asa] → [a=sspitzer, a=asa]
Comment 34•22 years ago
|
||
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.
Assignee | ||
Comment 35•22 years ago
|
||
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•22 years ago
|
||
People are reporting bug 204022 fixed today. It looks like this was checked in
this morning. Resolve Fixed?
Comment 37•22 years ago
|
||
Yes, I believed this was checked in since other depend bugs listed have been
fixed in today's trunk.
Comment 38•22 years ago
|
||
Checkin was completed earlier this morning. Marking verfied.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 39•22 years ago
|
||
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
Assignee | ||
Comment 40•22 years ago
|
||
Nobody is perfect, maybe I should have wait and fix the separators too, but then
it would not make 1.4 ...
Comment 41•22 years ago
|
||
a=adt Please check into the Mozilla 1.4 branch and add the keyword fixed1.4
Assignee | ||
Comment 42•22 years ago
|
||
This was fixed right before branching for 1.4, so it's already on the branch.
Comment 43•21 years ago
|
||
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
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•