Closed Bug 322989 Opened 19 years ago Closed 18 years ago

Add Bookmark UI

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 2 alpha2

People

(Reporter: bugs, Assigned: mozilla)

References

Details

Attachments

(2 files, 3 obsolete files)

The Add Bookmark UI within the browser window. Includes one-click-add and advanced add.
Severity: normal → blocker
Priority: -- → P1
*** Bug 317848 has been marked as a duplicate of this bug. ***
Target Milestone: --- → Firefox 2 alpha1
Apply from browser/components/places.
Attachment #210303 - Flags: review?(bugs)
Status: NEW → ASSIGNED
Target Milestone: Firefox 2 alpha1 → Firefox 2 alpha2
Joe - does this depend on tabbrowser-watcher?
No--I ended up implementing the same approach that tabbedbrowserwatcher uses in the few places that I needed it.  If we end up integrating tabbedbrowserwatcher, we'll be able to replace a few bits of this code with calls to it.
Comment on attachment 210303 [details] [diff] [review]
Initial implementation of Places bookmark properties panel.

Thanks for your hard work on this Joe - this is great progress. Here are some comments:

>+PlacesBrowserShim._updateControlStates = 
>+function PBS__updateControlStates() {
>+  var feedButton = document.getElementById("places-subscribe");
>+  var starButton = document.getElementById("places-star");
>+
>+  if (this._bms.isBookmarked(this._getCurrentLocation)) {

You need to invoke this like a function - this._getCurrentLocation() ... or maybe better make the function into a property:

 get _currentLocation() {
   return ...;
 },

...

if (this._bms.isBookmarked(this._currentLocation)) {
 ...
}

As you have it, this code won't work. 

For the record, this functionality concerns me from a PLT regression point of view. We will need to watch PLT numbers on tinderbox closely after places is enabled. If this causes a slowdown, the benefit offered by showing bookmarked-ness as page loads occur probably isn't worth the hit. 

I'm just letting you know that I was experimenting with style in this file and I've decided I don't like the one I picked. I'm going to eventually move it back to the functions-defined-within-object-literal format, which supports nicer syntactic sugar like get/set.

>+    starButton.label = "Star (On)";

I think we want to change this terminology before checking it in. If not in the UI (because Star is at least compact enough not to take up too much space before we get an icon) then in the code. 


>+  var tagArea = document.getElementById("tagbox");
>+
>+  while (tagArea.childNodes.length > 0) {
>+    tagArea.removeChild(tagArea.childNodes[0]);
>+  }

use tagArea.hasChildNodes() and tagArea.firstChild instead. 


>+/**
>+ * This method is called to exit the Bookmark Properties panel.
>+ *
>+ * @param aSaveChanges boolean, should be true if changes performed while
>+ *                     the panel was active should be saved
>+ */
>+
>+PlacesBrowserShim.hideBookmarkProperties = 
>+function PBS_hideBookmarkProperties(aSaveChanges) {
>+  if (aSaveChanges) {
>+    var titlebox = document.getElementById("edit-titlebox");
>+    this._bms.setItemTitle(this._currentURI, titlebox.value);
>+  }

nit: from here on out we'll only prefix member variables and private functions, with "_" - parameters need no "a" prefix. Same nit for all your other functions. 

>+var MAX_DEPTH = 6;

This is a constant, defined in the global scope. That's sort of risky. Why not make it a property of the shim object for a start, and also add a comment explaining what it does. 

>+  <toolbaritem id="urlbar-container">
>+    <button id="places-star" label="Star (Off)" oncommand="PlacesBrowserShim.onStarClick();" align="start"/>
>+    <button id="places-subscribe" label="Feed" tooltiptext="Subscribe"/>

Put these strings into a locale DTD file. If there is not one for this XUL file, create one. We have enough strings here now that this should all be localized. 

Can I have one more patch that addresses these comments and then we can check in?
Attachment #210303 - Flags: review?(bugs) → review-
Attachment #210303 - Attachment is obsolete: true
Attachment #210938 - Flags: superreview?(bryner)
Attachment #210938 - Flags: review?
Comment on attachment 210938 [details] [diff] [review]
Patch containing first pass at bookmark creation/filing UI

r=ben@mozilla.org
Attachment #210938 - Flags: review? → review+
Attachment #210938 - Flags: superreview?(bryner)
Attachment #210938 - Flags: superreview+
Attachment #210938 - Flags: review?(annie.sullivan)
Attachment #210938 - Flags: review+
Comment on attachment 210938 [details] [diff] [review]
Patch containing first pass at bookmark creation/filing UI

>+  onBookmarkButtonClick: function PBS_onBookmarkButtonClick() {
>+    var urlbar = document.getElementById("urlbar");
>+    this._currentURI = this._makeURI(urlbar.value);
>+    if (this._bms.isBookmarked(this._currentURI)) {
>+      this.showBookmarkProperties();
>+    } else {
>+      this._bms.insertItem(this._bms.bookmarksRoot, this._currentURI, -1);
>+      this._updateControlStates();
>+    }
>+  },

>+    var title = this._bms.getItemTitle(this._makeURI(urlbar.value));
>+    titlebox.value = title;

What happens if the user has edited the urlbar and the this._makeURI() returns null?
I think you want to bail early in these cases, or maybe provide a helpful error message?


(from showBookmarkProperties())
>+    var query = this._hist.getNewQuery();
>+    query.setFolders([this._bms.placesRoot], 1);
>+    var options = this._hist.getNewQueryOptions();
>+    options.setGroupingMode([Ci.nsINavHistoryQueryOptions.GROUP_BY_FOLDER], 1);
>+    options.excludeItems = true;
>+
>+    this._result = this._hist.executeQuery(query, options);

You could execute this query once in init(), and set this._result.root.containerOpen
to false.  When you need to access it, set this._result.containerOpen to true,
and then false after you're finished with it.  (Looks like you're already doing that part.)
This would be faster than requerying in the case where no bookmarks had changed.


>+  _populateTags:
>+  function PBS__populateTags (container, depth, parentElement, elementDict) {
>+    if (!container.containerOpen) {
>+      throw new Error("The containerOpen property of the container parameter should be set to true before calling populateTags(), and then set to false again afterwards.");
>+    }

This should probably use the ASSERT() function in controller.js instead of throw.
Attachment #210938 - Flags: review?(annie.sullivan) → review+
Depends on: 327167
This patch replaces the in-window bookmark properties panel with a modal dialog implementation.  It also contains fixes for the panel width issue (326677) and the  livemark assignability issue (326675).
Attachment #211926 - Flags: superreview?(bugs)
Attachment #211926 - Flags: review?(annie.sullivan)
Comment on attachment 211926 [details] [diff] [review]
Patch converting Add Bookmark panel to modal dialog


>   _makeURI: function PBS__makeURI(urlString) {
>+    if (!urlString) {
>+      throw new Error("_makeURI called with an undefined string parameter");
>+    }
>     return this._ios.newURI(urlString, null, null);
>   },

Is controller.js accessible from this context?  If not, I think you should make it accessible, and use ASSERT() instead of throw.

>       // If we can't alter it, no use showing it as an option.
>-      if (childFolder.childrenReadOnly) {
>+
>+      // childFolder.childrenReadOnly currently returns wrong answer for
>+      // livemarks (joe@retrovirus.com 2006-02-14)
>+        //      if (childFolder.childrenReadOnly) {
>+      if (this._bms.getFolderReadonly(childFolder.folderId)) {

This will probably change again, you should use nodeIsReadOnly() in controller.js.
Attachment #211926 - Flags: review?(annie.sullivan) → review+
Comment on attachment 211926 [details] [diff] [review]
Patch converting Add Bookmark panel to modal dialog

>+    content/browser/places/bookmarkproperties.css        (content/bookmarkproperties.css)
>+    content/browser/places/bookmarkproperties.xul        (content/bookmarkproperties.xul)

use interCaps for file names. 

>+<?xml version="1.0"?>
>+
>+<?xml-stylesheet href="chrome://browser/content/places/bookmarkproperties.css"?>
>+
>+<!DOCTYPE window SYSTEM "chrome://browser/locale/places/places.dtd">
>+
>+<window id="bookmarkproperties" title="&bookmark.property.panel.title;"

Is this a dialog? Use <dialog>. It supplies buttons which you can use or hide. At any rate, any dialog box should use <dialog>

>+		<label id="places-link" value="&cmd.show_bookmarks.label;" onclick="PlacesBrowserShim.hideBookmarkProperties(true, document); window.close(); PlacesBrowserShim.showBookmarks();"/>

This is a fair amount of code to stick in an inline event handler. Put it into a function. Also, change this to a button for now. Since visual appearance isn't our primary concern at this stage, accessibility should still be. Use oncommand. 

>+		<button label="&cmd.delete_bookmark.label;" oncommand="PlacesBrowserShim.deleteBookmark(); PlacesBrowserShim.hideBookmarkProperties(false, document); window.close()" align="end"/>
>+		<button label="&cmd.hide_bookmark_properties.label;" oncommand="PlacesBrowserShim.hideBookmarkProperties(true, document); window.close();"/>

>Index: content-shim/browserShim.js
>   _makeURI: function PBS__makeURI(urlString) {
>+    if (!urlString) {
>+      throw new Error("_makeURI called with an undefined string parameter");
>+    }

What annie said about ASSERT.

>+  showBookmarkProperties: function PBS_showBookmarkProperties() {
>+    this._currentTitle = this._bms.getItemTitle(this._currentURI);
>+    window.open("chrome://browser/content/places/bookmarkproperties.xul", "bookmarkproperties", "width=600,height=400,chrome,dependent,modal=yes,dialog=yes,resizable=yes");
>   },

What is 600x400? You don't need to provide those properties. Consider using openDialog instead of window.open, that way you can omit dialog=yes. openDialog is also more flexible for future changes (it allows parameters to be passed to the window). 

Let's have one final patch that addresses all of this.
Attachment #211926 - Flags: superreview?(bugs) → superreview-
*** Bug 327368 has been marked as a duplicate of this bug. ***
Attachment #211926 - Attachment is obsolete: true
Attachment #212046 - Flags: superreview?(bugs)
Comment on attachment 212046 [details] [diff] [review]
Patch changing places panel to modal dialog, incorporating feedback from last patch

>Index: jar.mn
>+    content/browser/places/bookmarkProperties.css        (content/bookmarkProperties.css)
>+    content/browser/places/bookmarkProperties.xul        (content/bookmarkProperties.xul)

I realize I forgot to point this out earlier:

.css files that contain theme matter (styles, colors etc) should go into the skin dir (see skin-win lines in jar.mn below). Move the file there instead and load using chrome://browser/skin/places/bookmarkProperties.css ...

You don't need to request review again for this correction, but attach a diff of your changes to this bug before checking in. 

sr=ben@mozilla.org with this change. 

The other thing I'd like to see but not necessarily for this checkin (you can do it as part of ongoing work) is to make the code in the popup window depend less on the parent window context. Rather, the parent window should tell the child what's going on. This makes the dependencies cleaner and clearer. It will also probably be required for reuse of any of this for bookmark properties.
Attachment #212046 - Flags: superreview?(bugs) → superreview+
This patch is the same as the previous one (212046) except that the bookmarkProperties.css file is now in skin-win/ instead of content/ as requested.  The contents of this patch were checked into trunk and MOZILLA_1_8_BRANCH.
Attachment #212046 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
*** Bug 327433 has been marked as a duplicate of this bug. ***
*** Bug 327474 has been marked as a duplicate of this bug. ***
Bookmarks are added forcibly under bookmarks.
can not select a folder where to add.
     You'll excuse my naiveté - PLEASE...  but this whole Bugzilla thing you people have gotten off on makes absolutely NO sense to the simply common folk such as myself.  Bugzilla is obviously designed for that secret society of programmer types who sit in coffee shops sending text messaging (another thing I have yet to uncover the workings of) each other.
     Which is probably why my Mozila Firefox "BOOKMARK THIS PAGE"  thingee >STILL<  doesn't work after reviewing at least thirty of your knowledge base item numbers...  most of which said simply, they were duplicates of others.

     Not to suggest I know how you/all ought to do your business of course, but wouldn’t it be a whole lot simpler just to 
  a> have an empty box on the “bug query” where someone like me - a Firefox unskilled-in-the-language-of-the-programming-gods Aficionado - could simply type in his problem (which, in my case as I’ve mentioned, is that the GXX DXXX FREEKIN  “BOOKmark this Page” thing don’t work). . . 
              whereupon, 
  b> your Mozila server sends back to me, a web page (once I hit enter), that states clearly and succinctly
          “HERE’S HOW YOU SOLVE THE PROBLEM DUMMY!”
(meaning of course, that you don’t forget to provide me solution here ok?)

Thank you for your attention to this BUG...
Michael...  a friend of Melinda (who is over in the corner pulling her hair out cause I talked her into installing Mozila in the first place).
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.

Attachment

General

Creator:
Created:
Updated:
Size: