Last Comment Bug 586947 - Places File Bookmark (Ctrl+D) needs improvement badly
: Places File Bookmark (Ctrl+D) needs improvement badly
Status: RESOLVED FIXED
: regression
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: x86 Windows 7
: -- normal with 2 votes (vote)
: seamonkey2.1b2
Assigned To: Philip Chee
:
Mentors:
Depends on: SMPlacesBMarks 573487 587343
Blocks: 590105 605786 605788 606151
  Show dependency treegraph
 
Reported: 2010-08-13 05:14 PDT by Philip Chee
Modified: 2010-11-23 02:40 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP Patch 1.0. proposed enhancement. (6.71 KB, patch)
2010-08-13 05:44 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Screenshot of WIP (40.13 KB, image/png)
2010-08-14 09:18 PDT, Philip Chee
no flags Details
Patch v1.1 (6.45 KB, patch)
2010-08-16 10:48 PDT, Philip Chee
iann_bugzilla: review+
Details | Diff | Splinter Review
Screenshot of Patch 1.1. (17.99 KB, image/png)
2010-08-16 10:55 PDT, Philip Chee
kairo: feedback-
Details
Patch v1.2. (6.96 KB, patch)
2010-08-26 08:35 PDT, Philip Chee
neil: superreview-
Details | Diff | Splinter Review
Screenshot of Patch v1.2 (29.59 KB, image/png)
2010-08-26 11:12 PDT, Philip Chee
no flags Details
Patch v2.0 Use the bookmark properties dialog. (8.45 KB, patch)
2010-09-19 13:34 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Patch v2.1 fix nits. (9.73 KB, patch)
2010-10-13 09:58 PDT, Philip Chee
neil: review+
Details | Diff | Splinter Review
Patch v2.2 use aResizeable instead of aMinimalUI. (10.59 KB, patch)
2010-10-18 08:08 PDT, Philip Chee
neil: review-
Details | Diff | Splinter Review
Patch v2.1.1 Without resizeable changes. r=Neil (8.70 KB, patch)
2010-10-20 03:57 PDT, Philip Chee
philip.chee: review+
neil: superreview+
Details | Diff | Splinter Review

Description Philip Chee 2010-08-13 05:14:24 PDT
From http://forums.mozillazine.org/viewtopic.php?p=9744353#p9744353

I find File Bookmark (Ctrl+D) to be FAR less useful.

1) Non-movable dialog displays upper right of browser window. I often have my download manager's capture window at just that location. So the one overlays the other.
2) dialog is small
3) dialog is hardly conducive to editing on the fly
4) even if you were to edit, you can only edit the Name:
5) you CANNOT manipulate the Location: at all

IMO the entire reason to use Ctrl+D is to be able to quickly & easily edit a bookmark to be saved. You can no longer do that. And if you cannot do that, then why even use Ctrl+D. You can just use Ctrl+Shift+D (Bookmark This Page) & be done with it.

For me, just about every time I Ctrl+D, it ends up being a 2-step (or more) process. First Ctrl+D, then I need to Ctrl+B (to open Bookmark Manager) so that I can then edit the entry to where I wanted it to be in the first place.

Example: http://forums.informaction.com/viewtopic.php?f=6&t=4157&sid=c07902855734745a3e8802d7cf4eef35

I'm viewing this thread. Hmm, great post, think I'll bookmark it. Looky there, the URL has that dumb sid= tacked on the end. Now that shouldn't be there. Ctrl+D. Uh? Duh? How do I edit the Location: ? How do I get rid of sid?

Screw it. Ctrl+D done. (Or not.) Ctrl+B, find the bookmark I just saved, then edit it. Dumb.

Storing username/password in the Location:. I do it all the time. Not the smartest thing perhaps, but it works for me. Often I need to reference or copy the un/pw.

Ctrl+D. There it is. Location: shows, http://therube:trythis@forums.mozillazine.org/. Just what I wanted to see. Now very simple for me to copy/paste 't h e r u ...". Ooops, wait a minute, Location: is no longer there? I can't see my un/pw. Now I have to Ctrl+B.
Comment 1 Philip Chee 2010-08-13 05:44:04 PDT
Created attachment 465626 [details] [diff] [review]
WIP Patch 1.0. proposed enhancement.

> 1) Non-movable dialog displays upper right of browser window. I often have my
> download manager's capture window at just that location. So the one overlays
> the other.
> 2) dialog is small
> 3) dialog is hardly conducive to editing on the fly
> 4) even if you were to edit, you can only edit the Name:
> 5) you CANNOT manipulate the Location: at all

This patch fixes issues #2 to #5.
I don't know why the panel isn't draggable. Or rather I can drag the panel via the titlebar but once I release the mouse it jumps back to the original position.

Also see Firefox Bug 418864 (Bookmark contextual dialog is not resizable) for a long flame war when users upgrading ran into the new bookmarks panel.
Comment 2 Philip Chee 2010-08-13 05:51:21 PDT
Question for Enn:

> I don't know why the panel isn't draggable. Or rather I can drag the panel via
> the titlebar but once I release the mouse it jumps back to the original
> position.
Comment 3 Neil Deakin 2010-08-13 06:51:50 PDT
(In reply to comment #2)
> > I don't know why the panel isn't draggable. Or rather I can drag the panel via
> > the titlebar but once I release the mouse it jumps back to the original
> > position.

That's the correct behaviour for a panel with level="parent". The panel maintains its anchored position relative to its parent. For this reason, using a titlebar with level="parent" isn't particularly useful.

You should never change the level attribute unless you know why you are doing so. Do you?
Comment 4 Philip Chee 2010-08-13 08:31:39 PDT
> You should never change the level attribute unless you know why you are doing
> so. Do you?
Initially I didn't have any level attribute and the panel wasn't draggable via the titlebar with the same symptoms. So with or without level the panel isn't draggable at the moment. Any suggestions?
Comment 5 Philip Chee 2010-08-13 08:34:42 PDT
OK level="top" worked. However I have "one or more text fields" and the MDC article on <panel> recommends that top be avoided in this situation.
Comment 6 Neil Deakin 2010-08-13 08:38:30 PDT
That's odd. Is this Windows only?
Comment 7 Philip Chee 2010-08-13 08:41:50 PDT
Sorry I'm on windows 7, I can't test on other platforms. The docs say that "parent" is the default on Windows so, without specifying a level= how do I get dragging to work?
Comment 8 Neil Deakin 2010-08-13 08:48:36 PDT
The default level is 'floating' if a titlebar is used, so it should work. A testcase would be good here and I could investigate further.
Comment 9 Philip Chee 2010-08-14 09:18:49 PDT
Created attachment 466007 [details]
Screenshot of WIP

Attaching screenshot of WIP
Comment 10 Robert Kaiser 2010-08-14 10:20:46 PDT
Ouch, that screenshot looks like yet another of those heavy and large windows I'm so glad we are trying to get rid of with the new approach!
Comment 11 Philip Chee 2010-08-14 11:22:04 PDT
> Ouch, that screenshot looks like yet another of those heavy and large windows
> I'm so glad we are trying to get rid of with the new approach!

It's a regression (multiple regressions). Didn't you read comment 0? Firefox has done a great job dumbing down the UI, we don't need to compete at that level. Also do read Bug 418864. Do we want a repeat of that?

Phil
Comment 12 Robert Kaiser 2010-08-14 11:29:54 PDT
(In reply to comment #11)
> Firefox has done a great job dumbing down the UI

In this case, I call that cleaning up UI. And both the Firefox places team and me know it can be even smoother and nicer (in this case, I'd e.g. really like to see use of the arrow panels or what they are called), but IMHO the current panel is a huge improvement over the ugly dialog we had before.

> Also do read Bug 418864. Do we want a repeat of that?

Ah, good, something to dupe to! :P

Seriously, I didn't even look at the patch, but the screenshot is IMHO ugly as hell and I would veto that being the default UI, with one reason being that it's way too large, for example, one other being the title.
Comment 13 Philip Chee 2010-08-14 21:24:37 PDT
It sure looks much better than the tiny squidged up Firefox panel you copied over!  Many Firefox users were dismayed at the loss of usability and functionality. Simplifying is one thing, removing needed functionality is just silly.
Comment 14 Philip Chee 2010-08-16 10:48:49 PDT
Created attachment 466364 [details] [diff] [review]
Patch v1.1

Changes since the last patch:

1. More compact UI: Folder tree is collapsed by default. Initial width is 32em.
2. Moved the anchor back to the right of the urlbar.
3. Resizer moved to bottom left.
4. The tags in the tag selector box are now inline to make the expanded tag box more compact.

   <panel id="editBookmarkPanel"
+          close="true"
+          level="top"
Temporarily set level to top in order to test draggability. Will remove later.
Comment 15 Philip Chee 2010-08-16 10:55:07 PDT
Created attachment 466367 [details]
Screenshot of Patch 1.1.

More compact UI.
Comment 16 Jens Hatlak (:InvisibleSmiley) 2010-08-16 11:19:12 PDT
I also feel this is a regression (just overlooked it until now). I don't think each and every aspect of Philip's proposal is really needed but this is what bugs me about the new dialog:
- position (top right? Are you kidding me? I have a big screen!)
- not movable
- not resizable
- folder list is collapsed by default and state changes are not saved
- Tags expander opens a huge space for what I consider to be rarely used to that extent
- cannot define a Keyword anymore (I use that heavily and see no sense in having to hunt down the newly created bookmark just to set it!)
- cannot change the address

What I care less about:
- Description. I'm not using that field at all, and doubt that many do.
- Load this bookmark in the sidebar. Seems to be advanced.

Positive:
- allows to remove the bookmark if it already exists (needs the star icon or similar to unlock its full potential, though)

My current File Bookmark in SeaMonkey 2.0 is about 60% of my 22" screen (width + height) so that I can see a large part of the bookmarks folder structure. I like that it's an ordinary window dialog which can be repositioned, resized, and which keeps that state across invocations.

If this turns out to become a matter of preference we should probably either implement an alternative File Bookmark dialog or create an extension. I'd prefer the former since I guess many people will be just as annoyed as Philip and me about this and having to maintain an extension is always an extra burden.

(In reply to comment #15)
> Created attachment 466367 [details]
> Screenshot of Patch 1.1.
> 
> More compact UI.

Having the resizer down left looks just wrong, at least for non-RTL locales.
Comment 17 therube 2010-08-16 11:33:55 PDT
Well I wasn't going to post (here), just to post a "me too", but have to say, I agree wholeheartedly with Jens.  (My comments, which basically mirror, are in mozillazine & cZ.)
Comment 18 Robert Kaiser 2010-08-17 12:03:24 PDT
Comment on attachment 466367 [details]
Screenshot of Patch 1.1.

That's definitely better, though I still am not too fond of the title bar, I think making it an arrow panel and adding the bookmarks icon to point the arrow to should do it. Also, the title itself is probably not ideal as IIRC, we only show the panel right now when adding a bookmark, for editing, you get a separate properties window right now, AFAIK.
Comment 19 Robert Kaiser 2010-08-22 15:10:37 PDT
Filed bug 589601 and a patch for the bookmarking button in the URL bar, this together with arrow panels (bug 554937) should make this panel feel much better.

Together with your work on at least the resizing, possibly more, I hope a lot more people will be able to like this panel.

For the resizing, we should make sure the size persists across calls, so those people who want it larger do always get it larger, and those who want it nicely compact will have it kept compact.
Comment 20 Ian Neal 2010-08-24 05:13:21 PDT
Comment on attachment 466364 [details] [diff] [review]
Patch v1.1

>-    gEditItemOverlay.initPanel(this._itemId,
>-                               { hiddenRows: ["description", "location",
>-                                              "loadInSidebar", "keyword"] });
>+    gEditItemOverlay.initPanel(this._itemId);
I'm happy with keyword and location to be shown, not sure about the other two, if we're trying to replicate the old dialog then should the destination be expanded (unhidden) as well?

>+    //gEditItemOverlay.toggleFolderTreeVisibility();
Nit: not needed?

No changes needed for css of Mac?

r=me with those addressed.
Comment 21 therube 2010-08-24 07:02:43 PDT
Few thoughts.

While I don't use the Description field, some sites pre-populate it, so it would be nice to know that potentially that data could be saved in the bookmark too.

The Remove Bookmark button, does it belong there at the top like that?  Would seem to be more appropriate to be bottom, left justified (& grayed out till such time as the bookmark has actually been save)?

Duplicates.  There needs to be some mechanism to easily save duplicate bookmarks from within a File Bookmarks dialog.
Comment 22 Robert Kaiser 2010-08-24 08:13:26 PDT
(In reply to comment #21)
> While I don't use the Description field, some sites pre-populate it, so it
> would be nice to know that potentially that data could be saved in the bookmark
> too.

Write an add-on for that, I guess that 98% of users probably never want or need the description field.

> The Remove Bookmark button, does it belong there at the top like that?

So that you don't need too large mouse movements when you want to remove the bookmark for the site you visited. Click the bookmark button, click the remove button appearing right below it, done.

> Duplicates.  There needs to be some mechanism to easily save duplicate
> bookmarks from within a File Bookmarks dialog.

I disagree, the file/edit panel doesn't need that, and the Bookmarks Manager, as well as proxy-icon-drag functionality allow that to be done for the rare cases where it's needed.
If you want the function for filing dupes in that panel, write an add-on.
Comment 23 Peter B. Shalimoff 2010-08-25 15:13:52 PDT
I was pretty happy with the old dialog (pre-Places one). I would be the happiest man in the world if the dialog had the "Description" field as in the bookmark properties dialog in the Bookmark Manager UI. Then when I saw the new dialog for the first time, my first thought was: OMFG. WHAT. THE. ****. IS. THIS?! Unmovable, unresizable, disappearing on a mouse scroll/outside click, even without the "Location" field? Then I wondered: is it the same thing Firefox users are doomed to deal with every day, or is this brilliant solution exclusive for SeaMonkey? Now I'm going to install Firefox to find this out.
Comment 24 Peter B. Shalimoff 2010-08-25 15:28:21 PDT
And by the way, on Windows, going from left to right, we (the users) want the "OK" button before the "Cancel" button, not the vice versa.
Comment 25 Peter B. Shalimoff 2010-08-25 15:57:53 PDT
Yep, the same dialog in Firefox, except the buttons are in the right order (at least in 3.6.8). Aliens go home.
Comment 26 Philip Chee 2010-08-26 08:35:44 PDT
Created attachment 469472 [details] [diff] [review]
Patch v1.2.

This patch:

1. makes the panel resizable and draggable.
1a. The resizer is in the bottom left corner because KaiRo didn't like it in the bottom right corner.
1b. The titlebar exists to make the panel draggable. Enn is working on a trunk patch to make panels draggable without a titlebar. Once that works we can remove the titlebar. In the meantime I've reclaimed some vertical space by moving the "Title" label into the titlebar.

2. The expanded Tags selector row is now more compact using an inline style.
3. There are no mac specific styles in this patch because editBookmarkOverlay.css isn't forked.

Changes since the previous patch:

1. The description and loadInSidebar rows are hidden.
2. Removed the close button on the panel title.
3. The panel size is persisted. I couldn't persist the position because the panel popup code forces the panel to anchor to the right edge of the urlbar.

4. display: inline; here works.

> +#editBMPanel_tagsSelector > listrows > listboxbody {
+  display: inline;
+} 

But causes the following error. I forgot what the fix for this is.

Warning: XUL box for listrows element contained an inline listboxbody child, forcing all its children to be wrapped in a block.
Source file: chrome://navigator/content/navigator.xul

5. I don't know why the background url isn't working. The DOMi shows tha the computed style is the rtl resizer but visually it is still the ltr resizer image.

> +/* Toolkit stripe is missing a rule for bottomleft resizers */
+resizer.bottomleft {
+/* XXXRatty:  this doesn't work! background: url ("chrome://global/skin/icons/resizer-rtl.png") no-repeat; */
> +  -moz-transform: scaleX(-1);
+}
Comment 27 Robert Kaiser 2010-08-26 09:21:40 PDT
Comment on attachment 466367 [details]
Screenshot of Patch 1.1.

OK, as people don't listen to my comments, I'll f- this.
Comment 28 Robert Kaiser 2010-08-26 09:25:47 PDT
Comment on attachment 469472 [details] [diff] [review]
Patch v1.2.

>+          noautohide="true"
>+          titlebar="normal"

I still disagree with both those lines strongly, including the implied draggability.

I like the resizer and persisting of size, though.-

> #editBookmarkPanelContent {
>   min-width: 23em;
>+  width: 32em;
> }

As it can be resized anyhow, I don't think there's any reason to change the default.

>+  -moz-transform: scaleX(-1);

Nice solution!
Comment 29 Philip Chee 2010-08-26 11:12:03 PDT
Created attachment 469530 [details]
Screenshot of Patch v1.2

Requested by therube
Comment 30 neil@parkwaycc.co.uk 2010-08-29 16:24:32 PDT
(In reply to comment #18)
> That's definitely better, though I still am not too fond of the title bar, I
> think making it an arrow panel and adding the bookmarks icon to point the arrow
> to should do it. Also, the title itself is probably not ideal as IIRC, we only
> show the panel right now when adding a bookmark, for editing, you get a
> separate properties window right now, AFAIK.
I think Bookmarks/File Bookmark... should open the properties window.

On the other hand, the fast bookmarking button probably should use this panel.
Comment 31 BlueAppleSlushie 2010-08-29 18:28:02 PDT
It would increase usability/discoverability to have useful default text in the tags and keyword fields.  Firefox has the text 'Separate tags with commas' in the tags field (Some might otherwise assume space is the separator as I did).  I propose 'For quick access from address bar' as the default text for the keyword field.

I hope not to crush Kairo's hope for a clean interface with the next proposal :)
This assumes that the Folder and Tags entries do not persist their expanded state.  Would it be possible to have the expand/hide button become a split button or add a small button just to the right of it when in the expanded state to pin it in the expanded state?  This would allow people who feel crippled by the simplified default view control without cluttering the view for people who expand those entries occasionally (without being pinned, the entry would be in the hidden state next time the dialog is accessed).
Comment 32 Robert Kaiser 2010-08-30 12:10:27 PDT
(In reply to comment #31)
> It would increase usability/discoverability to have useful default text in the
> tags and keyword fields.  Firefox has the text 'Separate tags with commas' in
> the tags field (Some might otherwise assume space is the separator as I did). 
> I propose 'For quick access from address bar' as the default text for the
> keyword field.

Actually, what they have there is probably a placeholder text, detectable as a user by being displayed in a different ("deactivated") color than normal text entries there, and by going away when you click in the textbox. If it's that and we are missing those, that's surely a bug and should be filed independently.
The subject of the bug right here is largely misleading as it implies to be a "do everything" bug, which are ones we should not have. Every bug should have a clear, measurable target. So, please, even if this stays due to the interesting and probably good work being done here, file anything going farther than the patch here as additional followup bugs with very specific and clear measurable targets.
Comment 33 neil@parkwaycc.co.uk 2010-09-01 03:12:51 PDT
Comment on attachment 469472 [details] [diff] [review]
Patch v1.2.

I think the panel is almost* right for the URLbar icon case (bug 589601) and should stay that way. But we need the full add bookmark UI somewhere, and so far my best idea is to make Ctrl+D open the New Bookmark dialog.

* Will follow up in a separate bug
Comment 34 Philip Chee 2010-09-19 13:34:43 PDT
Created attachment 476672 [details] [diff] [review]
Patch v2.0 Use the bookmark properties dialog.

> I think the panel is almost* right for the URLbar icon case (bug 589601) and
> should stay that way. But we need the full add bookmark UI somewhere, and so
> far my best idea is to make Ctrl+D open the New Bookmark dialog.
Take 2: Use the bookmark properties dialog.

> +      if ("foldertree" in dialogInfo) {
> +        if (this._element("folderTreeRow").collapsed) {
> +          setTimeout(function() gEditItemOverlay.toggleFolderTreeVisibility(), 10);
I don't know why a setTimeout() is needed here but otherwise I get a "gEditItemOverlay is null" which doesn't make any sense as it is already defined.

>              <tree id="editBMPanel_folderTree"
>                    class="placesTree"
>                    type="places"
> +                  treelines="true"
Turn on treelines for the folder picker folder tree. I still haven't found a way of making the folder tree flex when the dialog is resized despite liberally sprinkling "flex" all over.

> +    var dialogURL = "chrome://communicator/content/bookmarks/bm-props.xul";
> +    var features = "centerscreen,chrome,modal,resizable=yes";
> +    this._getCurrentActiveWin().openDialog(dialogURL, "",  features, info);
> +    return ("performed" in info && info.performed);
I didn't use return this._showBookmarkDialog() here as I wanted the dialog to be resizable and to use the full dialog instead of the minimal dialog. I could have modified _showBookmarkDialog() but this would have affected other callers.
Comment 35 neil@parkwaycc.co.uk 2010-09-26 13:15:49 PDT
Comment on attachment 476672 [details] [diff] [review]
Patch v2.0 Use the bookmark properties dialog.

You forgot to actually hook up your code to the UI...

>+      if ("foldertree" in dialogInfo) {
>+        if (this._element("folderTreeRow").collapsed) {
>+          setTimeout(function() gEditItemOverlay.toggleFolderTreeVisibility(), 10);
I don't like the tree being uncollapsed by default, especially on a timeout.

>+    var parent = aParent != undefined ?
>+                 aParent : PlacesUtils.bookmarksMenuFolderId;
aParent || PlacesUtils.bookmarksMenuFolderId>+    PlacesUIUtils.showAddBookmarkAsUI(uri, title, description, insertPoint);
Can't you use showAddBookmarkUI instead?

>+    info.hiddenRows = aHiddenRows ? aHiddenRows : ["loadInSidebar"];
[aHiddenRows ||]

>+#editBMPanel_tagsSelector {
>+  max-height: 5em;
>+}
>+
>+#editBMPanel_tagsSelector > listrows > listboxbody {
>+  display: inline-block;
>+}
Not really relevant to this bug?
Comment 36 Philip Chee 2010-10-13 09:58:41 PDT
Created attachment 482885 [details] [diff] [review]
Patch v2.1 fix nits.

> neil@parkwaycc.co.uk      2010-09-26 13:15:49 PDT
> 
> Comment on attachment 476672 [details] [diff] [review]
> Patch v2.0 Use the bookmark properties dialog.
> 
> You forgot to actually hook up your code to the UI...
Fixed.

> >+      if ("foldertree" in dialogInfo) {
> >+        if (this._element("folderTreeRow").collapsed) {
> >+          setTimeout(function() gEditItemOverlay.toggleFolderTreeVisibility(), 10);
> I don't like the tree being uncollapsed by default, especially on a timeout.
Removed.

> >+    var parent = aParent != undefined ?
> >+                 aParent : PlacesUtils.bookmarksMenuFolderId;
> aParent || PlacesUtils.bookmarksMenuFolderId
Fixed.

> >+   PlacesUIUtils.showAddBookmarkAsUI(uri, title, description, insertPoint);
> Can't you use showAddBookmarkUI instead?
Now using showAddBookmarkUI().

> >+    info.hiddenRows = aHiddenRows ? aHiddenRows : ["loadInSidebar"];
> [aHiddenRows ||]
Fixed by removing this.

> >+#editBMPanel_tagsSelector {
> >+  max-height: 5em;
> >+}
> >+
> >+#editBMPanel_tagsSelector > listrows > listboxbody {
> >+  display: inline-block;
> >+}
> Not really relevant to this bug?
I'll file a follow up bug.
Comment 37 neil@parkwaycc.co.uk 2010-10-13 13:21:04 PDT
Comment on attachment 482885 [details] [diff] [review]
Patch v2.1 fix nits.

>-    var features;
>-    if (aMinimalUI)
>-      features = "centerscreen,chrome,modal,resizable=yes";
>-    else
>-      features = "centerscreen,chrome,modal,resizable=no";
>+    var features = "centerscreen,chrome,modal,resizable=yes";
Why this change?
Comment 38 Philip Chee 2010-10-15 02:28:09 PDT
>>-    var features;
>>-    if (aMinimalUI)
>>-      features = "centerscreen,chrome,modal,resizable=yes";
>>-    else
>>-      features = "centerscreen,chrome,modal,resizable=no";
>>+    var features = "centerscreen,chrome,modal,resizable=yes";
> Why this change?

As it stands the minimal UI is resizable but the full UI isn't and we are calling the full UI here. One of the complaints that led to this bug is that the addbookmark dialog/panel isn't resizeable. I thought to make the full UI resizable. This affects showAddLivemarkUI(), showItemProperties(), and showAddFolderUI() as well.

I could add a third parameter to _showBookmarkDialog() to force resizability if you perfer?
Comment 39 Philip Chee 2010-10-15 03:22:26 PDT
> I could add a third parameter to _showBookmarkDialog() to force resizability if
> you perfer?

With the File Bookmark is resizeable, since the UI is shared, showAddLivemarkUI(), showItemProperties(), and showAddFolderUI(), these other dialogs will open with the persisted width of the File Bookmark dialog with no way of resizing if it is too squished.
Comment 40 neil@parkwaycc.co.uk 2010-10-15 09:59:02 PDT
(In reply to comment #38)
> As it stands the minimal UI is resizable but the full UI isn't and we are
> calling the full UI here. One of the complaints that led to this bug is that
> the addbookmark dialog/panel isn't resizeable. I thought to make the full UI
> resizable. This affects showAddLivemarkUI(), showItemProperties(), and
> showAddFolderUI() as well.
> 
> I could add a third parameter to _showBookmarkDialog() to force resizability if
> you perfer?
Given bug 580662 comment #12 (right at the end) maybe aMinimalUI should be renamed to aResizable?

Possibly unrelated:
The old bookmark dialog was clever and only persisted the size of the flexible elements. So if they were hidden then the dialog would default to normal size.
Comment 41 Philip Chee 2010-10-18 08:08:36 PDT
Created attachment 483976 [details] [diff] [review]
Patch v2.2 use aResizeable instead of aMinimalUI.

> Given bug 580662 comment #12 (right at the end) maybe aMinimalUI should be
> renamed to aResizable?
Ooo. Yes. Fixed.

> Possibly unrelated:
> The old bookmark dialog was clever and only persisted the size of the flexible
> elements. So if they were hidden then the dialog would default to normal size.
Urm. I would prefer to do that in a follow-up bug if you don't mind. Or rather someone else can. Places bookmarks gives me a headache.
Comment 42 neil@parkwaycc.co.uk 2010-10-18 09:31:17 PDT
Comment on attachment 482885 [details] [diff] [review]
Patch v2.1 fix nits.

The resizability issue is hurting my brain. Can we leave that bit for now?
Comment 43 Igor Velkov 2010-10-18 09:40:11 PDT
Description and keyword fields are missing.
Comment 44 Philip Chee 2010-10-18 09:55:09 PDT
> Description and keyword fields are missing.
Lets get this bug landed and decide on those fields in a followup bug. Neil has enough problems reviewing the current patches.
Comment 45 Philip Chee 2010-10-18 09:56:22 PDT
> The resizeability issue is hurting my brain. Can we leave that bit for now?
In v2.2 the only thing I did for _showBookmarkDialog() was to rename aMinimalUI to aResizable. Is that OK? Or do you mean to keep the CTRL-D dialog non-resizeable?
Comment 46 neil@parkwaycc.co.uk 2010-10-18 14:23:00 PDT
Comment on attachment 482885 [details] [diff] [review]
Patch v2.1 fix nits.

>-    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 47 Philip Chee 2010-10-20 03:57:20 PDT
Created attachment 484649 [details] [diff] [review]
Patch v2.1.1 Without resizeable changes. r=Neil

>>-    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.
Removed.
Comment 48 neil@parkwaycc.co.uk 2010-10-20 04:08:58 PDT
Comment on attachment 484649 [details] [diff] [review]
Patch v2.1.1 Without resizeable changes. r=Neil

Thanks.
Comment 49 Philip Chee 2010-10-20 05:16:50 PDT
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/428648891029
Resolving as FIXED. Follow-up bugs will be filed for the remaining issues.
Comment 50 Robert Kaiser 2010-10-21 03:48:50 PDT
Seesh. Now I'll need to file a bug about the ugly large width of this panel. :(
Comment 51 Peter Lairo 2010-11-23 02:40:30 PST
(In reply to comment #44)
> > Description and keyword fields are missing.
> Lets get this bug landed and decide on those fields in a followup bug. Neil has
> enough problems reviewing the current patches.

There's already a bug for that: 89001. It was erroneously closed. Please REOPEN it.

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