405010 - make column changes in places/library (session) persistent

Status: VERIFIED FIXED

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

Description

[(mostly gone) XtC4UaLL [:xtc4uall]]

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

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

adding Fx2 paritiy as the first mentioned non-deluxe-expectation is working with FF2.

Comment 7

[Mike Connor [:mconnor]]

Marco, can you whip up a patch for this?  Should be reasonably easy to persist the columns, unless I'm missing something.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch

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 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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)

Comment 10

[Asaf Romano (gone)]

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.

Comment 11

[Asaf Romano (gone)]

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".

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(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.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(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

Comment 14

[Asaf Romano (gone)]

Can you ask Alex about this? I think he wanted "show all bookmarks" to explicitly select the "all bookmarks" :) folder

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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?

Comment 16

[Asaf Romano (gone)]

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 17

[Alex Faaborg [:faaborg] (Firefox UX)]

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

switching ui-r to beltzner

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: stripped patch

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

Comment 19

[Asaf Romano (gone)]

Comment on attachment 307184 [details] [diff] [review]
stripped patch

r=mano

Comment 20

[Dietrich Ayala (:dietrich)]

hm, doesn't apply. marco, can you unrot this?

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: unbitrot

Comment 22

[Reed Loden [:reed]]

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

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 24

[Aakash Desai [:aakashd]]

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.

Comment 25

[Gervase Markham [:gerv]]

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