Last Comment Bug 605786 - Places File Bookmark dialog should be resizeable.
: Places File Bookmark dialog should be resizeable.
Status: RESOLVED FIXED
: regression
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: seamonkey2.10
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
: 606151 716878 (view as bug list)
Depends on: 586947 739049
Blocks: 733687
  Show dependency treegraph
 
Reported: 2010-10-20 05:21 PDT by Philip Chee
Modified: 2012-03-25 04:36 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Proof of concept (7.59 KB, patch)
2012-02-29 14:12 PST, neil@parkwaycc.co.uk
jh: review+
Details | Diff | Review
Proposed patch (10.74 KB, patch)
2012-03-04 11:01 PST, neil@parkwaycc.co.uk
jh: review+
Details | Diff | Review
Addressed review comments (10.74 KB, patch)
2012-03-04 13:11 PST, neil@parkwaycc.co.uk
neil: review+
Details | Diff | Review
Addressed review comments [Checkin: Comments 21 and 23] (12.60 KB, patch)
2012-03-04 13:59 PST, neil@parkwaycc.co.uk
jh: review+
bugspam.Callek: approval‑comm‑aurora+
Details | Diff | Review

Description Philip Chee 2010-10-20 05:21:46 PDT
Splitting off resizeability issues to a separate bug.

From Bug 586947 Comment 46:

>>-    var features;
>>-    if (aMinimalUI)
>>-      features = "centerscreen,chrome,modal,resizable=yes";
>>-    else
>>-      features = "centerscreen,chrome,modal,resizable=no";
>>-    this._getCurrentActiveWin().openDialog(dialogURL, "",  features, aInfo);
>>+    var features = "centerscreen,chrome,modal,resizable=yes";
> Sorry, but I think the resizability issue is too complicated to include in this
> bug, so I'd prefer to go with this patch, but without this resizable change.
Comment 1 Philip Chee 2010-11-08 07:11:54 PST
*** Bug 606151 has been marked as a duplicate of this bug. ***
Comment 2 Jens Hatlak (:InvisibleSmiley) 2011-06-16 13:40:39 PDT
I think it should not only be resizable but must also persist its size across sessions.

Furthermore, the area below the Folder drop-down must IMO have a flex (take any space that is gained by resizing the window) and its visibility (expanded state) must be made persistent. If this needs to go to its own bug, fine. But I think it's a vital part to make this dialog usable again.

I'm adding the regression keyword because all of this is a huge UX regression compared to SM 2.0, and at the same time should be relatively easy to fix.
Comment 3 Philip Chee 2011-06-16 21:09:44 PDT
> at the same time should be relatively easy to fix
Assigned => InvisibleSmiley then?
Comment 4 Jens Hatlak (:InvisibleSmiley) 2011-06-20 14:21:34 PDT
(In reply to comment #3)
> > at the same time should be relatively easy to fix
> Assigned => InvisibleSmiley then?

If only I knew what I was doing. It's easy to describe what I want, but hard to make it work.

I succeeded in making the window resizable (not taking different dialog types into account yet), persist width and height, and to make the area below the Folder drop-down flexible (mostly by adding flex="1" to <vbox id="editBookmarkPanelContent">), but failed to make the folder selection <tree> flexible (it currently has a fixed height). Maybe the <grid>/<vbox> combination is preventing it (until SM 2.0, the <tree> was inside a <vbox> but not a <grid>) from working the way I would expect it. Adding flex="1" to various elements using DOMI didn't help much, either. :-/

Furthermore I failed to make the expanded state persistent (tried persist="collapsed" in bm-props.xul). Either I don't get it or this needs a JS workaround.

I begin to wonder whether it's me or XUL that is too dumb. If trouble continues I might just create an add-on that reverts the UI back to what SM 2.0 had (plus tags support of course)...
Comment 5 Jens Hatlak (:InvisibleSmiley) 2012-01-10 14:21:19 PST
*** Bug 716878 has been marked as a duplicate of this bug. ***
Comment 6 Phoenix 2012-01-10 14:35:10 PST
Can't find this bug with search...
Anyway, this regression is one of the reasons, holding part of users on 2.0.x, see http://forums.mozillazine.org/viewtopic.php?f=3&t=2364173 for example
Comment 7 Jens Hatlak (:InvisibleSmiley) 2012-01-10 14:54:12 PST
(In reply to Phoenix from comment #6)
> Anyway, this regression is one of the reasons, holding part of users on
> 2.0.x, see http://forums.mozillazine.org/viewtopic.php?f=3&t=2364173 for
> example

I know... I asked Ratty whether he can port the OpenBook add-on which should help in the mean time.

I use the following Stylish user style to at least get a taller list (when expanded):

#editBMPanel_folderTree {
min-width: 340px ! important;
min-height: 400px ! important;
}
#editBookmarkPanelContent {
min-width: 300px ! important;
}
Comment 8 neil@parkwaycc.co.uk 2012-02-29 14:12:20 PST
Created attachment 601758 [details] [diff] [review]
Proof of concept

This is just to make the toggles interact with resizing; I didn't try to persist the height but that's easy to add if it is wanted. Presumably in the .jsm we would just unconditionally make the dialog resizable?

The ugly outerWidth hack exists because I couldn't come up with a sane way of predicting what sizeToContent() would decide to size the width to.
Comment 9 Jens Hatlak (:InvisibleSmiley) 2012-03-02 13:57:52 PST
Comment on attachment 601758 [details] [diff] [review]
Proof of concept

[Sorry, no time for an actual review yet. :-( But thanks for giving this a try, it surely is appreciated!]

(In reply to neil@parkwaycc.co.uk from comment #8)
> I didn't try to persist the height but that's easy to add if it is wanted.

It is! :-) Super bonus points for persisting the expanded state, too (otherwise it'll be a follow-up issue).

I only did very few testing now (on Linux) and it happened to me once that the Description field resized instead of the tree area. Need to try and reproduce when that happens.

> Presumably in the .jsm we would just unconditionally make the dialog resizable?

Err, which .jsm?

I noticed, though, that even with the patch the dialog is not resizable at all on Windows; I guess we need to adjust the calling code for that. Could you check/do that please?
Comment 10 neil@parkwaycc.co.uk 2012-03-02 14:02:36 PST
(In reply to Jens Hatlak from comment #9)
> (In reply to comment #8)
> > I didn't try to persist the height but that's easy to add if it is wanted.
> It is! :-) Super bonus points for persisting the expanded state, too
> (otherwise it'll be a follow-up issue).
I guess that's possible, but I didn't look at that part of the code...

> I only did very few testing now (on Linux) and it happened to me once that
> the Description field resized instead of the tree area. Need to try and
> reproduce when that happens.
Well, I did make the Description field resizable too, in case you had collapsed the tree and tags.

> > Presumably in the .jsm we would just unconditionally make the dialog resizable?
> Err, which .jsm?
PlacesUIUtils.jsm

> I noticed, though, that even with the patch the dialog is not resizable at
> all on Windows; I guess we need to adjust the calling code for that. Could
> you check/do that please?
The calling code? Oh, you mean the .jsm ;-) See my question above!
Comment 11 Jens Hatlak (:InvisibleSmiley) 2012-03-02 14:14:48 PST
(In reply to neil@parkwaycc.co.uk from comment #10)
> Well, I did make the Description field resizable too, in case you had
> collapsed the tree and tags.

I doubt many people use the Description field, and it is scroll bars enabled, so I'd be OK with keeping it fixed-height (say, three lines). Having the possibility to resize the dialog to a custom width should already take care of most lengthy descriptions (assuming they are rather continuous text than multi-line).

> > > Presumably in the .jsm we would just unconditionally make the dialog resizable?
> > Err, which .jsm?
> PlacesUIUtils.jsm

Then yes, always resizable (unless Stefan objects for possible Mac-specific reasons, but for now let's assume he agrees).
Comment 12 Jens Hatlak (:InvisibleSmiley) 2012-03-03 08:55:19 PST
Comment on attachment 601758 [details] [diff] [review]
Proof of concept

Review of attachment 601758 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the nit addressed and an unconditional resizable=yes flag in the .jsm (I'll leave it to you whether to have a single "var features =" line or inline it and kill the variable altogether).

Thanks a ton!

If you choose to go one step further and try to come up with a patch (replacing this one or an extra one) that also remembers the tree expanded state (including the twisty) that would be very much appreciated! :-) Otherwise, if you choose to close this bug, please open a follow-up to take care of that aspect. Thanks!

Note: The Bookmark This Group of Tabs option seems to not remember the dialog size, or reset it. As long as the tree expanded state is not saved, that doesn't really matter, though.

Once this has baked a bit on trunk we might consider backporting to branches (well, right now probably not to Beta anymore).

::: suite/common/bookmarks/bm-props.js
@@ +401,5 @@
>          }
>          break;
>  
> +      case "resize":
> +        ["editBMPanel_folderTreeRow", "editBMPanel_tagsSelectorRow", "editBMPanel_descriptionRow"].forEach(function(e) {

Please wrap (and indent accordingly) after the second array element.
Comment 13 neil@parkwaycc.co.uk 2012-03-04 11:01:53 PST
Created attachment 602746 [details] [diff] [review]
Proposed patch
Comment 14 Jens Hatlak (:InvisibleSmiley) 2012-03-04 12:46:03 PST
Comment on attachment 602746 [details] [diff] [review]
Proposed patch

Review of attachment 602746 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the extra removals.

Please file a follow-up bug for remembering the tree expanded state as explained in comment 12. Thanks!

::: suite/common/src/PlacesUIUtils.jsm
@@ -643,5 @@
>     * @return true if any transaction has been performed, false otherwise.
>     */
> -  _showBookmarkDialog: function PUIU__showBookmarkDialog(aInfo, aMinimalUI) {
> -    var dialogURL = aMinimalUI ?
> -                    "chrome://communicator/content/bookmarks/bm-props2.xul" :

You're removing the last (and only) reference to bm-props2.xul here. I'm fine with that (FF removed the minimal UI in bug 696159, which we're planning to do in our bug 712533), but please remove the obsolete file here (including the jar.mn change) before it gets lost.

BTW the method signature change is OK, too. I considered add-on compat for a second, but current FF code has two other (incompatible) parameters so this change doesn't make it worse.
Comment 15 neil@parkwaycc.co.uk 2012-03-04 13:08:52 PST
(In reply to Jens Hatlak from comment #14)
> You're removing the last (and only) reference to bm-props2.xul here. I'm
> fine with that (FF removed the minimal UI in bug 696159, which we're
> planning to do in our bug 712533), but please remove the obsolete file here
> (including the jar.mn change) before it gets lost.
Actually it's an override, not an actual file, but I do need the jar.mn change.
Comment 16 neil@parkwaycc.co.uk 2012-03-04 13:11:46 PST
Created attachment 602759 [details] [diff] [review]
Addressed review comments

With jar.mn change.
Comment 17 Jens Hatlak (:InvisibleSmiley) 2012-03-04 13:14:37 PST
Comment on attachment 602759 [details] [diff] [review]
Addressed review comments

Wrong upload? Well, as long as you push the right thing... ;-)
Comment 18 neil@parkwaycc.co.uk 2012-03-04 13:59:19 PST
Created attachment 602764 [details] [diff] [review]
Addressed review comments [Checkin: Comments 21 and 23]

I blame bug 724881 or any similar bugs that have previously been WONTFIXed...
Comment 19 Jens Hatlak (:InvisibleSmiley) 2012-03-07 00:41:46 PST
Neil, I filed that follow-up for you. Anything else keeping you from checking in and closing this one?

Mind you, next Tuesday is Aurora uplift day. Ideally this one should land before that. Then we can either go for Aurora approval before or Beta approval after that day.
Comment 20 neil@parkwaycc.co.uk 2012-03-07 01:09:02 PST
(In reply to Jens Hatlak from comment #19)
> Anything else keeping you from checking in and closing this one?
I often don't have enough free time on Mondays or Tuesdays.
Comment 21 neil@parkwaycc.co.uk 2012-03-07 13:06:35 PST
Pushed changeset cbfab6350b17 to comm-central.
Comment 22 Jens Hatlak (:InvisibleSmiley) 2012-03-08 10:13:26 PST
Comment on attachment 602764 [details] [diff] [review]
Addressed review comments [Checkin: Comments 21 and 23]

[Approval Request Comment]
Regression caused by (bug #):                          Bug 498596
User impact if declined:                               Continued bad UX
Testing completed (on m-c, etc.):                      Works with latest nightly
Risk to taking this patch (and alternatives if risky): None I know
String changes made by this patch:                     None
Comment 23 Jens Hatlak (:InvisibleSmiley) 2012-03-08 15:09:23 PST
Comment on attachment 602764 [details] [diff] [review]
Addressed review comments [Checkin: Comments 21 and 23]

http://hg.mozilla.org/releases/comm-aurora/rev/da05932da851
Comment 24 therube 2012-03-16 17:16:18 PDT
To note:  Width & location persist, but height does not.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120316 Firefox/13.0a2 SeaMonkey/2.10a2
Comment 25 therube 2012-03-16 17:22:35 PDT
> <NeilAway>	therube: ideally bug 733687 should fix that too

So I'll leave it at that :-).

Note You need to log in before you can comment on or make changes to this bug.