Remove the keywords column from the Library

RESOLVED FIXED in Firefox 40

Status

()

Toolkit
Places
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: mak, Assigned: ttaubert)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
mozilla40
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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+
(Reporter)

Updated

2 years ago
Blocks: 1094818
(Assignee)

Comment 1

2 years ago
Marco, can you please estimate this?
Flags: needinfo?(mak77)
(Reporter)

Comment 2

2 years ago
I'm calling it easy cause should be mostly code removal, 3 because the code sucks.
Points: --- → 3
Flags: needinfo?(mak77)
(Reporter)

Updated

2 years ago
Blocks: 1140395
No longer blocks: 1141547

Comment 3

2 years ago
Does this mean we can't do keyword searches from the urlbar once this is removed?!?
(Reporter)

Comment 4

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

Comment 5

2 years ago
Well-scoped code deletion, yo. Can I get a mentor and a DXR link for this?
(Assignee)

Comment 6

2 years ago
(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)
(Reporter)

Comment 7

2 years ago
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@bonardo.net
Flags: needinfo?(mak77)
Whiteboard: [good next bug][lang=js]

Comment 8

2 years ago
> 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.
(Reporter)

Comment 9

2 years ago
(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...
(Assignee)

Comment 10

2 years ago
(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.
(Reporter)

Updated

2 years ago
Flags: needinfo?(philip.chee)

Comment 11

2 years ago
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)
(Reporter)

Comment 12

2 years ago
(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

2 years ago
(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.
(Reporter)

Updated

2 years ago
Points: 3 → 2

Updated

2 years ago
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
(Assignee)

Updated

2 years ago
Mentor: mak77@bonardo.net
Whiteboard: [good next bug][lang=js]
(Assignee)

Comment 14

2 years ago
Created attachment 8600882 [details] [diff] [review]
0001-Bug-1145063-Remove-the-keywords-column-from-the-Libr.patch

Tests seem to be running fine.
Attachment #8600882 - Flags: review?(mak77)
(Reporter)

Comment 15

2 years ago
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+
(Assignee)

Comment 16

2 years ago
(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

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/920b1705a242
https://hg.mozilla.org/mozilla-central/rev/920b1705a242
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox40: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40

Updated

2 years ago
Depends on: 1172366

Comment 19

2 years ago
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

2 years ago
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)

Updated

2 years ago
Flags: needinfo?(mak77)
(Reporter)

Comment 21

2 years ago
(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.
(Reporter)

Comment 23

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

Comment 24

2 years ago
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

Comment 26

2 years ago
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.
(Reporter)

Comment 27

2 years ago
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

2 years ago
(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

4 months ago
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?
(Reporter)

Comment 30

4 months ago
yes, bug 648398.
You need to log in before you can comment on or make changes to this bug.