Closed Bug 405010 Opened 12 years ago Closed 12 years ago

make column changes in places/library (session) persistent

Categories

(Firefox :: Bookmarks & History, defect, P3, minor)

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta5

People

(Reporter: xtc4uall, Assigned: mak)

References

Details

(Whiteboard: [Fx2-parity])

Attachments

(1 file, 2 obsolete files)

STR:
- open places/library
- notice the given columns "name", "tags", "location"
- add one (or more) columns (e.g. "keyword")
- close the window and/or firefox in total

expected:
after reopening places/library the above chosen "extra-column" (e.g. "keyword") should show up.

expected_deluxe:
after restarting Firefox *and* places/library the above chosen "extra-column" (e.g. "keyword") should show up.

actual result:
the chosen column is gone
Flags: blocking-firefox3?
adding Fx2 paritiy as the first mentioned non-deluxe-expectation is working with FF2.
Whiteboard: [Fx2-parity]
Assignee: nobody → mano
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Target Milestone: Firefox 3 → Firefox 3 M10
Duplicate of this bug: 405200
Duplicate of this bug: 381764
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Duplicate of this bug: 408280
Duplicate of this bug: 410224
Target Milestone: Firefox 3 beta3 → ---
Duplicate of this bug: 418770
Marco, can you whip up a patch for this?  Should be reasonably easy to persist the columns, unless I'm missing something.
Assignee: mano → mak77
Attached patch patch (obsolete) — Splinter Review
this patch makes the following changes:

- columns are draggable (Bug 405017)
- column position and visibility is persistent (Bug 405010)
- column sorting is persistent (Bug 405108)
- last selected folder in the left pane is persistent (Bug 329853)

I notice a problem however: in the Library we allow to D&D only if the view is not sorted. If we persist the sorting status across sessions we will most probably have complain from the users that the D&D is not working. 

IMHO, we should persist the sorting order in the Library when a user moves through folders, but not persist it across sessions, this way when the user opens the Library the D&D is correctly working, and he will soon understand the difference between a sorted view and an unsorted one.

There already a related bug that i have still to fill, in a sorted view the drop should not be allowed (while it appear possible atm, a regression?).

Mano what do you think about this?
Comment on attachment 306530 [details] [diff] [review]
patch

mano i'm asking review to have your suggestions on this, see the previous comment (if you also have hints on the changes it would be great)
Attachment #306530 - Flags: review?(mano)
Do we also disable d&d in Fx2 in this case? I believe we should allow d&d to sub folders even when the view is sorted.

That said, I don't think there's much value in persisting the sort method anyway. If we cannot get sensible d&d support when the view is sorted, it'd be fine not to persist sorting for the time being.
I think you should take out "- last selected folder in the left pane is persistent (Bug 329853) " for now, this doesn't play so well with "Show All Bookmarks" and "Show All History".
(In reply to comment #10)
> Do we also disable d&d in Fx2 in this case? I believe we should allow d&d to
> sub folders even when the view is sorted.

it's bogus there too, the drop is done, then the view gest sorted again (so the dropped item go to another position). Probably we should allow dropping from a folder to another folder (dropping at the end), but not moving item into the same folder since that is not reliable. Or we can retain the FX2 bogus behaviour.

> That said, I don't think there's much value in persisting the sort method
> anyway. If we cannot get sensible d&d support when the view is sorted, it'd be
> fine not to persist sorting for the time being.

i agree to not retain that across sessions, but in the same session a user visit a folder and set location sorting. Then he moves on another folder and has to set sorting again. In FX2 we are retaining sorting in this case.

> I think you should take out "- last selected folder in the left pane is
> persistent (Bug 329853) " for now, this doesn't play so well with "Show All
> Bookmarks" and "Show All History".

It should not have any problem with them, since it gives priority to window arguments, has a default to AllBookmarks, can accept both query identifers (names) than item ids. So when you click Show All bookmarks the position is remembered (except when the position is invalid, so it go back to All Bookmarks), while when you click Show All history the "history" argument takes priority over remembered one. FX2 remember last position in bookmarks organizer.
(In reply to comment #11)
mmh rethinking i see what you say here, Show All Bookmarks should always go to All Bookmarks... we don't have a menuitem to open the Library, as in FX2... we could add such an item in Tools menu? still the patch would be good since show all bookmarks would have an argument, open Library would not.

we could take the change but not activate the feature (leaving the argument in show all bookmarks menuitem), in future it would be easier implement persistent folder
Can you ask Alex about this? I think he wanted "show all bookmarks" to explicitly select the "all bookmarks" :) folder
Comment on attachment 306530 [details] [diff] [review]
patch

Alex, Show All Bookmarks should always point to "All Bookmarks" or should remember the last selected folder as in FX2? Or do we need a new menu item to open the Library to the last selected position?
Attachment #306530 - Flags: ui-review?(faaborg)
I prefer not check in this code unelss we're using it.

As for persisting sorting, the fx2 behavior isn't desired, no. It would be nice if  you could get the "allow dropping at end" behavior working.
Comment on attachment 306530 [details] [diff] [review]
patch

switching ui-r to beltzner
Attachment #306530 - Flags: ui-review?(faaborg) → ui-review?(beltzner)
Attached patch stripped patch (obsolete) — Splinter Review
let's take it easy, this only contains:

- columns are draggable (Bug 405017)
- column position and visibility is persistent (Bug 405010)

will move other infos / patches /requests to the respective bugs
Attachment #306530 - Attachment is obsolete: true
Attachment #307184 - Flags: review?(mano)
Attachment #306530 - Flags: ui-review?(beltzner)
Attachment #306530 - Flags: review?(mano)
Status: NEW → ASSIGNED
Whiteboard: [Fx2-parity] → [Fx2-parity][has patch][needs review Mano]
Keywords: checkin-needed
Whiteboard: [Fx2-parity][has patch][needs review Mano] → [Fx2-parity][has patch][has review]
hm, doesn't apply. marco, can you unrot this?
Attached patch unbitrotSplinter Review
Attachment #307184 - Attachment is obsolete: true
Checking in browser/components/places/content/places.xul;
/cvsroot/mozilla/browser/components/places/content/places.xul,v  <--  places.xul
new revision: 1.120; previous revision: 1.119
done
Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.53; previous revision: 1.52
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [Fx2-parity][has patch][has review] → [Fx2-parity]
Target Milestone: --- → Firefox 3 beta5
Verified fixed with:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5pre) Gecko/2008030806 Minefield/3.0b5pre ID:2008030806

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008030804 Minefield/3.0b5pre ID:2008030804
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Test cases https://litmus.mozilla.org/show_test.cgi?id=4132 for the 3.0 test run and https://litmus.mozilla.org/show_test.cgi?id=6090 for 3.1 test run were updated with this regression bug Id.
Flags: in-litmus? → in-litmus+
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.