1421664 - Make SelectAsTag use tag queries instead of RESULTS_AS_TAG_CONTENTS

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P1)

Description

[Milind L (:milindl)]

Please see

https://bugzilla.mozilla.org/show_bug.cgi?id=1293445#c11

SelectAsTag needs to return a query selecting "place:tag1", "place:tag2" etc rather than "place:type=7&(...)".

Comment 1

[Milind L (:milindl)]

> I think we should not have an explicit queryType, queryType should be forced when parsing the query and finding a tag keyword.
> 
> For example here:
> https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/toolkit/components/places/nsNavHistoryQuery.cpp#1366
> https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/toolkit/components/places/nsNavHistoryQuery.cpp#1468
> https://searchfox.org/mozilla-central/source/toolkit/components/places/nsNavHistoryQuery.cpp#1141

A couple of doubts regarding this:

-> Tagged URLs which aren't also bookmarks may be an issue (though they are not very common I suppose?). 
More concretely, this will lead directly to the failure of some unit tests for example [1]

-> If I do decide to do that, I'm not sure how I'll do it as neatly as in the example, since mTags is in the query, while mQueryType is in options. I currently do it while converting queries to SQL string inside ConstructQueryString[2].

[1] https://searchfox.org/mozilla-central/source/toolkit/components/places/tests/queries/test_tags.js#146
[2] https://searchfox.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#2098

Comment 2

[Marco Bonardo [:mak]]

(In reply to Milind L (:milindl) from comment #1)
> -> Tagged URLs which aren't also bookmarks may be an issue (though they are
> not very common I suppose?). 
> More concretely, this will lead directly to the failure of some unit tests
> for example [1]

They should not be common, and our future plan moves the tagging api into the bookmarks api, so they should become non-existing soon-ish.
If some unit test is wrong (not adding a bookmark) it should be fixed.
 
> -> If I do decide to do that, I'm not sure how I'll do it as neatly as in
> the example, since mTags is in the query, while mQueryType is in options. I
> currently do it while converting queries to SQL string inside
> ConstructQueryString[2].

Ah hm, that's true, it was simpler with resultType because both were in the options object. The only point is indeed when the options object and the query objects are evaluated together. the system is over-engineered.
So yes, either ConstructQueryString, or even earlier in ExecuteQueries... It's a mess, sooner or later we'll rewrite all this query building code in js.

Comment 3

[Milind L (:milindl)]

Attachment: Bug 1421664 - Part 1: Force any tag queries to have query result type bookmarks.

Review commit: https://reviewboard.mozilla.org/r/204238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204238/

Comment 4

[Milind L (:milindl)]

Attachment: Bug 1421664 - Part 2: Fix SelectAsTag and add test.

Review commit: https://reviewboard.mozilla.org/r/204240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204240/

Comment 5

[Milind L (:milindl)]

The second patch has several issues, so I didn't mark it for review, I'll try finding them and fixing them as I go. Some issues I could notice:

* In the library, the old queries were accompanied by a "tag" icon, but new queries use a "gear" icon
* Fails a test added in Bug 524219, so probably a similar, but I've not pinpointed exactly what is happening.

Comment 6

[Marco Bonardo [:mak]]

it's likely there's code around that only acts on RESULT_AS_TAG_CONTENTS queries but should also be enabled for "tag=..." queries. Since we didn't used these queries deeply yet, those bugs have not been fixed during these years.
I'd suggest to look at the RESULT_AS_TAG_CONTENTS usage around and see what can be augmented to also handle "tag=..." queries.

Comment 7

[Marco Bonardo [:mak]]

I will probably look at the whole tags problem in the next weeks, starting from your patches and moving on from there.
If you have any updated versions of the patches please attach them before EOW. Thanks!

Comment 8

[Marco Bonardo [:mak]]

taking over the work since it now involves more complex fixes in our result code. Will coalesce changes to separate patches keeping the original blame for the changes.

Comment 9

[Marco Bonardo [:mak]]

Attachment: Bug 1421664 - 1 - Force any tag queries to have query result type bookmarks.

Review commit: https://reviewboard.mozilla.org/r/231178/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231178/

Comment 10

[Marco Bonardo [:mak]]

Attachment: Bug 1421664 - 2 - Change SelectAsTag to use place: tag queries.

Review commit: https://reviewboard.mozilla.org/r/231180/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231180/

Comment 11

[Marco Bonardo [:mak]]

Attachment: Bug 1421664 - 3 - Fix the UI and tests.

test_419731.js can be removed since it is already covered by browser_bookmarkProperties_editTagContainer.js

Review commit: https://reviewboard.mozilla.org/r/231182/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231182/

Comment 12

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4892f08b6f1778df0c587b12f420b7290ddc3ae8

Comment 13

[Marco Bonardo [:mak]]

The first 2 patches are Milind's patches that I have unbitrotted, the third patch adds the missing bits

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8962294 [details]
Bug 1421664 - 1 - Force any tag queries to have query result type bookmarks.

https://reviewboard.mozilla.org/r/231178/#review236568

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8962295 [details]
Bug 1421664 - 2 - Change SelectAsTag to use place: tag queries.

https://reviewboard.mozilla.org/r/231180/#review236570

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8962296 [details]
Bug 1421664 - 3 - Fix the UI and tests.

https://reviewboard.mozilla.org/r/231182/#review237144

I've given this an initial review/run, generally looking ok, but there's a couple of issues I think need looking at.

::: browser/components/places/content/editBookmark.js:631
(Diff revision 1)
> -      let guid = this._paneInfo.isTag
> -                  ? (await PlacesUtils.promiseItemGuid(this._paneInfo.itemId))
> -                  : this._paneInfo.itemGuid;
> -      await PlacesTransactions.EditTitle({ guid, title: newTitle }).transact();
> +      await PlacesTransactions.RenameTag({ oldTag, tag }).transact();
> +
> +      // If the node has a guid, then it's a copy of the tag query, and its url
> +      // should be updated to the new tag.
> +      if (guid) {
> +        await PlacesTransactions.EditUrl({ guid, url: "place:tag=" + tag }).transact();

I think we need to do some sort of block update on place:tag= queries here.

With the patches, if I create a copy of a tag query, and then go back and rename the original query (under the Tags container), then the newly created copy doesn't update and no longer points to any bookmarks.

::: toolkit/components/places/PlacesTransactions.jsm:1634
(Diff revision 1)
> + * Required Input Properties: oldTag, tag.
> + */
> +PT.RenameTag = DefineTransaction(["oldTag", "tag"]);
> +PT.RenameTag.prototype = {
> +  async execute({ oldTag, tag }) {
> +    // For now this is implemented y

nit: incomplete comment.

::: toolkit/components/places/PlacesUtils.jsm:755
(Diff revision 1)
>     * Determines whether or not a result-node is a tag container.
>     * @param   aNode
>     *          A result-node
>     * @returns true if the node is a tag container, false otherwise
>     */
>    nodeIsTagQuery: function PU_nodeIsTagQuery(aNode) {

It looks like getConcreteItemId needs updating for the changes as well. When running the library view, I'm seeing lots of:

ReferenceError: reference to undefined property "query"[Learn More] PlacesUtils.jsm:812:11
TypeError: aNode.query is undefined

in the browser console.

::: toolkit/components/places/nsNavHistory.cpp:1665
(Diff revision 1)
>  
>    // This allows sorting by date fields what is not possible with
>    // other history queries.
>    mHasDateColumns = true;
>  
> +  // TODO: This is likely wrong, since the tag name should probably be

Do we have a bug for this? If we don't I think we should, and we should reference it here in either case.

Comment 17

[Marco Bonardo [:mak]]

(In reply to Mark Banner (:standard8) from comment #16)
> With the patches, if I create a copy of a tag query, and then go back and
> rename the original query (under the Tags container), then the newly created
> copy doesn't update and no longer points to any bookmarks.

Yes, this is unfortunately expected, because doing otherwise it would be very expensive. To support that, we should search every open result for all the tag queries, for each query look if it's pointing to the tag you renamed, and update it. Considered we keep open results for menus, it would be a pain.
Imo, we should consider copying a tag container and then renaming a tag an edge-case, changing case is not a problem and it's the most common case. The user can always remove the broken query and copy the new tag.

Ideas are welcome, I honestly tried to do things magic, but after hours trying to design something simple, I decided to give up and consider copies of tag queries as independent entities. Indeed nodeIsTagQuery won't recognize a copy of a tag query as a tag query, on purpose. Doing otherwise adds lots of complication when the user edits the copy, for example changing the copy title was changing the tag, but other copies didn't follow that change, plus I may want to rename the copy to something else, but keep the tag name intact.

Comment 18

[Marco Bonardo [:mak]]

This also means that dropping into the copy of a tag container won't tag. I think for now we should reduce complexity for the refactoring, in the future we can always add bells and whistles.

Comment 19

[Marco Bonardo [:mak]]

Basically, the only way out I can think of is to make RenameTag find all the queries with the given tag, and call EditUrl on all of them.

Comment 20

[Mark Banner (:standard8)]

(In reply to Marco Bonardo [::mak] from comment #18)
> This also means that dropping into the copy of a tag container won't tag. I
> think for now we should reduce complexity for the refactoring, in the future
> we can always add bells and whistles.

The only issue I have with this, is that the copy looks the same as the original for the user (e.g. same icon etc), so this may be confusing that the can do it to one place and not to another.

Having said that, I suspect most users don't really copy tag queries that much, nor do the drag and drop into tags. So we're probably ok trying this, and we might just have to be prepared to add it back later.

(In reply to Marco Bonardo [::mak] from comment #17)
> Ideas are welcome, I honestly tried to do things magic, but after hours
> trying to design something simple, I decided to give up and consider copies
> of tag queries as independent entities. Indeed nodeIsTagQuery won't
> recognize a copy of a tag query as a tag query, on purpose. Doing otherwise
> adds lots of complication when the user edits the copy, for example changing
> the copy title was changing the tag, but other copies didn't follow that
> change, plus I may want to rename the copy to something else, but keep the
> tag name intact.

I think you're right, it is going to be hard to do this. My only thought like your comment 19 wrt finding all of the queries & editing them which is likely expensive.

So for now, lets try going with it as it is, and see what the feedback is.

Maybe we can add these to the list of things for UX to think about when they're looking at reorganising the library displays.

Comment 21

[Marco Bonardo [:mak]]

(In reply to Mark Banner (:standard8) from comment #20)
> The only issue I have with this, is that the copy looks the same as the
> original for the user (e.g. same icon etc), so this may be confusing that
> the can do it to one place and not to another.

It shouldn't, with my change the original should have the tag icon, the copy should have a normal query icon (the gear)

> Having said that, I suspect most users don't really copy tag queries that
> much, nor do the drag and drop into tags. So we're probably ok trying this,
> and we might just have to be prepared to add it back later.

Yes, that's my idea, we can always add stuff at a later time.

I will still see how hard would it be to make renameTag do the whole thing before resubmitting, it may simplify the dialog code a bit.

Comment 22

[Marco Bonardo [:mak]]

Comment on attachment 8962294 [details]
Bug 1421664 - 1 - Force any tag queries to have query result type bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231178/diff/1-2/

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 8962295 [details]
Bug 1421664 - 2 - Change SelectAsTag to use place: tag queries.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231180/diff/1-2/

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 8962296 [details]
Bug 1421664 - 3 - Fix the UI and tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231182/diff/1-2/

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 8962296 [details]
Bug 1421664 - 3 - Fix the UI and tests.

https://reviewboard.mozilla.org/r/231182/#review238082

::: browser/components/places/content/controller.js:178
(Diff revision 2)
>               !PlacesUtils.asQuery(this._view.result.root).queryOptions.excludeItems &&
>               this._view.result.sortingMode ==
>                   Ci.nsINavHistoryQueryOptions.SORT_BY_NONE;
>      case "placesCmd_show:info": {
>        let selectedNode = this._view.selectedNode;
> -      return selectedNode && PlacesUtils.getConcreteItemId(selectedNode) != -1;
> +      return selectedNode && PlacesUtils.nodeIsTagQuery(selectedNode) ||

This needs brackes around the or part (I see errors on the console when selecting items in the library, unless there's brackets).

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 8962296 [details]
Bug 1421664 - 3 - Fix the UI and tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231182/diff/2-3/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f8bb8f9998e
1 - Force any tag queries to have query result type bookmarks. r=mak
https://hg.mozilla.org/integration/autoland/rev/4b1ce6bf8518
2 - Change SelectAsTag to use place: tag queries. r=mak
https://hg.mozilla.org/integration/autoland/rev/147023fe220c
3 - Fix the UI and tests. r=standard8

Comment 28

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/7f8bb8f9998e
https://hg.mozilla.org/mozilla-central/rev/4b1ce6bf8518
https://hg.mozilla.org/mozilla-central/rev/147023fe220c