Closed Bug 1145063 Opened 9 years ago Closed 9 years ago

Remove the keywords column from the Library

Categories

(Toolkit :: Places, defect)

defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
mozilla40
Iteration:
40.3 - 11 May
Tracking Status
firefox40 --- fixed

People

(Reporter: mak, Assigned: ttaubert)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file)

There are very few keywords compared to number of bookmarks, sorting bookmarks by keywords is not really useful.

We should have a better keywords UI, that initially could be similar to tags UI in the Library, or we should just merge the UI with the search keywords UI.
But not here.

For now the keywords column is blocking migration to the new bookmarks API, thus, since sparse columns are not really useful, we are just removing it.
Flags: qe-verify-
Flags: firefox-backlog+
Blocks: 1094818
Marco, can you please estimate this?
Flags: needinfo?(mak77)
I'm calling it easy cause should be mostly code removal, 3 because the code sucks.
Points: --- → 3
Flags: needinfo?(mak77)
Blocks: 1140395
No longer blocks: 1141547
Does this mean we can't do keyword searches from the urlbar once this is removed?!?
nope, we are only removing the keywords column from the Library view. nothing more than that.
Well-scoped code deletion, yo. Can I get a mentor and a DXR link for this?
(In reply to Mike Hoye [:mhoye] from comment #5)
> Well-scoped code deletion, yo. Can I get a mentor and a DXR link for this?
Flags: needinfo?(mak77)
there's quite a bit to remove, hope I won't lose pieces:

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js#1243
1243       keyword:      { key: "KEYWORD",      dir: "ascending"  },

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.xul#393
393             <treecol label="&col.keyword.label;" id="placesContentKeyword" anonid="keyword" flex="1" hidden="true"
394                       persist="width hidden ordinal sortActive sortDirection"/>
395             <splitter class="tree-splitter"/>

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/treeView.js#521
COLUMN_TYPE_KEYWORD: 5,

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/treeView.js#539
539       case "keyword":
540         return this.COLUMN_TYPE_KEYWORD;

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/treeView.js#571
571       case Ci.nsINavHistoryQueryOptions.SORT_BY_KEYWORD_ASCENDING:
572         return [this.COLUMN_TYPE_KEYWORD, false];
573       case Ci.nsINavHistoryQueryOptions.SORT_BY_KEYWORD_DESCENDING:
574         return [this.COLUMN_TYPE_KEYWORD, true];

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/treeView.js#853
853     this._invalidateCellValue(aNode, this.COLUMN_TYPE_KEYWORD);

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/treeView.js#1447
1447       case this.COLUMN_TYPE_KEYWORD:
1448         if (PlacesUtils.nodeIsBookmark(node))
1449           return PlacesUtils.bookmarks.getKeywordForBookmark(node.itemId);
1450         return "";

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/treeView.js#1586
1586       case this.COLUMN_TYPE_KEYWORD:
1587         if (oldSort == NHQO.SORT_BY_KEYWORD_ASCENDING)
1588           newSort = NHQO.SORT_BY_KEYWORD_DESCENDING;
1589         else if (allowTriState && oldSort == NHQO.SORT_BY_KEYWORD_DESCENDING)
1590           newSort = NHQO.SORT_BY_NONE;
1591         else
1592           newSort = NHQO.SORT_BY_KEYWORD_ASCENDING;
1593 
1594         break;

Finally, in a separate patch or even a separate bug (likely better) we should get rid of the SORT_BY_KEYWORD code
http://mxr.mozilla.org/mozilla-central/search?string=SORT_BY_KEYWORD
and all the code supporting it.
Mentor: mak77
Flags: needinfo?(mak77)
Whiteboard: [good next bug][lang=js]
> Finally, in a separate patch or even a separate bug (likely better) we
> should get rid of the SORT_BY_KEYWORD code
> http://mxr.mozilla.org/mozilla-central/search?string=SORT_BY_KEYWORD
> and all the code supporting it.
Please don't remove the backend e.g. nsINavHistoryService.idl . SeaMonkey still uses these. Thanks.
(In reply to Philip Chee from comment #8)
> > Finally, in a separate patch or even a separate bug (likely better) we
> > should get rid of the SORT_BY_KEYWORD code
> > http://mxr.mozilla.org/mozilla-central/search?string=SORT_BY_KEYWORD
> > and all the code supporting it.
> Please don't remove the backend e.g. nsINavHistoryService.idl . SeaMonkey
> still uses these. Thanks.

ok. do you plan to drop the keywords column from your manager or do you plan to keep it? Honestly it is not very useful...
(In reply to Philip Chee from comment #8)
> Please don't remove the backend e.g. nsINavHistoryService.idl . SeaMonkey
> still uses these. Thanks.

Can we work together on having SeaMonkey not use it anymore? It feels wrong to keep it around for only a single consumer.
Flags: needinfo?(philip.chee)
There is still a keyword field right? Just in a different part of the Schema.

(In reply to Marco Bonardo [::mak] from comment #9)
> ok. do you plan to drop the keywords column from your manager or do you plan
> to keep it? Honestly it is not very useful...
I would like to keep it if it is not too much trouble for you. I can think of at least one use case: User knows that he has a keyword search somewhere in his bookmarks. Turns on the keyword column (tree column picker widget) and takes a quick eyeball while scrolling down. Or maybe: wants to look for all keyword searches.

(In reply to Tim Taubert [:ttaubert] from comment #10)
> Can we work together on having SeaMonkey not use it anymore?
Can a user search for a keyword in the Firefox Library/Bookmarks manager? That would help I think.

> It feels wrong to keep it around for only a single consumer.
<Tyrion>What if that consumer is "Firefox"?</Tyrion>

I did a quick peek via MXR (http://mxr.mozilla.org/mozilla-central/search?string=SORT_BY_KEYWORD) and honestly it doesn't seem like much code is involved.
Flags: needinfo?(philip.chee)
(In reply to Philip Chee from comment #11)
> There is still a keyword field right? Just in a different part of the Schema.

Yes, it's bound to uris, not to bookmarks.

> (In reply to Marco Bonardo [::mak] from comment #9)
> I would like to keep it if it is not too much trouble for you. I can think
> of at least one use case: User knows that he has a keyword search somewhere
> in his bookmarks. Turns on the keyword column (tree column picker widget)
> and takes a quick eyeball while scrolling down. Or maybe: wants to look for
> all keyword searches.

It's a valid use-case, but not for this UI. indeed I think we'll provide an alternative UI to manage keywords, maybe similar to the tags folder in the library. The best would be to merge it with the search keywords ui in preferences.

> (In reply to Tim Taubert [:ttaubert] from comment #10)
> > Can we work together on having SeaMonkey not use it anymore?
> Can a user search for a keyword in the Firefox Library/Bookmarks manager?
> That would help I think.

nope, and I don't think it will ever happen.

> I did a quick peek via MXR
> (http://mxr.mozilla.org/mozilla-central/search?string=SORT_BY_KEYWORD) and
> honestly it doesn't seem like much code is involved.

yes, but it adds complication, for example all the views must listen to keywords changes, when effectively it's rare and used only by the treeview for the simple use-case of "find all my keywords". As I said, it would be better to have a simple UI that just does that.
(In reply to Marco Bonardo [::mak] from comment #12)
> (In reply to Philip Chee from comment #11)

> yes, but it adds complication, for example all the views must listen to
> keywords changes, when effectively it's rare and used only by the treeview
> for the simple use-case of "find all my keywords". As I said, it would be
> better to have a simple UI that just does that.
If you can get the keyword UI up before removing the column I'd be grateful.
Points: 3 → 2
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
Mentor: mak77
Whiteboard: [good next bug][lang=js]
Tests seem to be running fine.
Attachment #8600882 - Flags: review?(mak77)
Comment on attachment 8600882 [details] [diff] [review]
0001-Bug-1145063-Remove-the-keywords-column-from-the-Libr.patch

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

r=me with the following fixed:

1. Please verify that enabling the keyword column in the Library, applying the patch, and launching the library, still works properly. We persist the columns state in localstore.rdf and I'd not want us to throw in the middle of columns initialization if some of them doesn't exist anymore. This should be managed by the tree code itself iirc so it should work, ideally. But it's better to be safe than sorry.

2. I'd expect browser/components/places/tests/browser/browser_sort_in_library.js to fail, since this line should be removed as well:
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/browser_sort_in_library.js#38
line 38 -- keyword: { key: "KEYWORD", dir: "ASCENDING" },

3. We also need to remove
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/places/places.properties#39
39 view.sortBy.1.keyword.label=Sort by Keyword
40 view.sortBy.1.keyword.accesskey=K
This is used by fillWithColumns, it goes through the columns and populates the "View/Order By" menupopup in the Library
(note you don't need to bump the versioning as requested by the localization comment cause this is a removal, not an update of its meaning)
Attachment #8600882 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #15)
> 1. Please verify that enabling the keyword column in the Library, applying
> the patch, and launching the library, still works properly. We persist the
> columns state in localstore.rdf and I'd not want us to throw in the middle
> of columns initialization if some of them doesn't exist anymore. This should
> be managed by the tree code itself iirc so it should work, ideally. But it's
> better to be safe than sorry.

Checked, works.

> 2. I'd expect
> browser/components/places/tests/browser/browser_sort_in_library.js to fail,
> since this line should be removed as well:
> http://mxr.mozilla.org/mozilla-central/source/browser/components/places/
> tests/browser/browser_sort_in_library.js#38
> line 38 -- keyword: { key: "KEYWORD", dir: "ASCENDING" },

Test is fine as we discovered on IRC.

> 3. We also need to remove
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/places/places.properties#39
> 39 view.sortBy.1.keyword.label=Sort by Keyword
> 40 view.sortBy.1.keyword.accesskey=K

Done.
https://hg.mozilla.org/mozilla-central/rev/920b1705a242
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1172366
I wonder if this has anything to do with keywords being all messed up in Firefox 40.  Exporting bookmarks as a json file shows different keywords than the bookmarks library, and changing keywords of bookmarks is no longer reliable.  Something got messed up.  But since this column is now removed, it is much, much harder to diagnose these issues.  The keyword column was very useful... did anyone consult the UI/UX team before completely removing it from the interface?
Is there a addon or something that would allow me to manage my keyword searches, since this is gone now in FF 40? How can I manage my keyword searches in the interim?

I have quite a few keyword searches and need to be able to manage them. Without the ability to search or sort keywords, how can I know what keywords are setup?
Flags: needinfo?(mak77)
Flags: needinfo?(mak77)
(In reply to matt.heerschap from comment #20)
> Is there a addon or something that would allow me to manage my keyword
> searches, since this is gone now in FF 40? How can I manage my keyword
> searches in the interim?
> 
> I have quite a few keyword searches and need to be able to manage them.
> Without the ability to search or sort keywords, how can I know what keywords
> are setup?

modulo a bug that will be fixed in next versions, keywords are still accessible through bookmarks, and you can search for %s in the library to find them.
Can you please undo this change? It makes searching for keywords in places (chrome://browser/content/places/places.xul) impossible. Due to conflicting keywords for search engines I want to remove keywords from bookmarks, but I can't find the associated bookmarks.
Anyway, there is no use in removing the column. Please add it back.
we will likely provide an alternative ui in the future, for now you can search bookmarks for "%" and that should return most keywords.
ehr, "%s"
"%s" doesn't find all search bookmarks.
The new UI is e.g. tracked here: https://bugzilla.mozilla.org/show_bug.cgi?id=261124
I agree with Christian that "%s" doesn't find all keywords. I myself have a number of keyworded bookmarks that serve as convenient aliases, so I don't have to type out a significant part of the URL before autocomplete kicks in (and is faster besides).

Another use case for the keyword column is the creation of a new keyword - previously, I can sort by keyword to know if a keyword is already in use, but now I have to manually look through all my bookmarks to see if what I want to assign has already been taken.
Bug 648398 is the closest thing to a tracking bug for a new UI.

It may also be easily implemented through an add-on in a week, if someone would be brave enough to use a couple APIs in PlacesUtils.keywords.
(In reply to Marco Bonardo [::mak] (Away Dec 12-20) from comment #27)
> It may also be easily implemented through an add-on in a week, if someone
> would be brave enough to use a couple APIs in PlacesUtils.keywords.

Which has been done here https://addons.mozilla.org/en-US/firefox/addon/revert-keyword-column/
With Revert Keyword Column, it is really great to have the Keyword column back. Any chance there will be a more permanent Keyword solution included in Firefox at some point?
yes, bug 648398.
See Also: → 1702895
You need to log in before you can comment on or make changes to this bug.