Make SelectAsTag use tag queries instead of RESULTS_AS_TAG_CONTENTS

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: milindl, Assigned: mak)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

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

Updated

2 years ago
Assignee: nobody → i.milind.luthra
Blocks: 1293445
Status: NEW → ASSIGNED
Reporter

Comment 1

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

Comment 2

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

Updated

2 years ago
Priority: -- → P2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Reporter

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

Last year
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.
Assignee: i.milind.luthra → mak77
Assignee

Updated

Last year
Attachment #8933300 - Flags: review?(mak77)
Assignee

Updated

Last year
Priority: P2 → P1
Whiteboard: [fxsearch]
Assignee

Updated

Last year
Depends on: 1446951
Assignee

Updated

Last year
Blocks: 1160193
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

Last year
Attachment #8933300 - Attachment is obsolete: true
Assignee

Updated

Last year
Attachment #8933301 - Attachment is obsolete: true
Assignee

Comment 13

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

Comment 14

Last year
mozreview-review
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
Attachment #8962294 - Flags: review?(mak77) → review+
Assignee

Comment 15

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

https://reviewboard.mozilla.org/r/231180/#review236570
Attachment #8962295 - Flags: review?(mak77) → review+

Comment 16

Last year
mozreview-review
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.
Attachment #8962296 - Flags: review?(standard8)
Assignee

Comment 17

Last year
(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.
Assignee

Comment 18

Last year
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.
Assignee

Comment 19

Last year
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.
(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.
Assignee

Comment 21

Last year
(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.
Assignee

Updated

Last year
Blocks: 1449939
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

Last year
mozreview-review
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).
Attachment #8962296 - Flags: review?(standard8) → review+
Comment hidden (mozreview-request)

Comment 27

Last year
Pushed by mak77@bonardo.net:
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
Assignee

Updated

Last year
Blocks: 1450245

Updated

Last year
Depends on: 1474164
Regressions: 1547801
You need to log in before you can comment on or make changes to this bug.