1145063 - Remove the keywords column from the Library

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Marco, can you please estimate this?

Comment 2

[Marco Bonardo [:mak]]

I'm calling it easy cause should be mostly code removal, 3 because the code sucks.

Comment 3

[Philip Chee]

Does this mean we can't do keyword searches from the urlbar once this is removed?!?

Comment 4

[Marco Bonardo [:mak]]

nope, we are only removing the keywords column from the Library view. nothing more than that.

Comment 5

[Mike Hoye [:mhoye]]

Well-scoped code deletion, yo. Can I get a mentor and a DXR link for this?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 7

[Marco Bonardo [:mak]]

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.

Comment 8

[Philip Chee]

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

Comment 9

[Marco Bonardo [:mak]]

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

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 11

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

Comment 12

[Marco Bonardo [:mak]]

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

Comment 13

[Philip Chee]

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

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1145063-Remove-the-keywords-column-from-the-Libr.patch

Tests seem to be running fine.

Comment 15

[Marco Bonardo [:mak]]

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)

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/920b1705a242

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/920b1705a242

Comment 19

[quality+bugzilla]

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?

Comment 20

[matt.heerschap]

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?

Comment 21

[Marco Bonardo [:mak]]

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

Comment 22

[Christian Stadelmann]

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.

Comment 23

[Marco Bonardo [:mak]]

we will likely provide an alternative ui in the future, for now you can search bookmarks for "%" and that should return most keywords.

Comment 24

[Marco Bonardo [:mak]]

ehr, "%s"

Comment 25

[Christian Stadelmann]

"%s" doesn't find all search bookmarks.
The new UI is e.g. tracked here: https://bugzilla.mozilla.org/show_bug.cgi?id=261124

Comment 26

[Justin Li]

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.

Comment 27

[Marco Bonardo [:mak]]

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.

Comment 28

[Scott R]

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

Comment 29

[tero gusto]

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?

Comment 30

[Marco Bonardo [:mak]]

yes, bug 648398.