adding kind and path attributes to suggest response object and using it in autocomplete #464

MananJethwani · 2021-03-17T06:15:18Z

@kelson42 @mgautierfr this resolves kiwix/kiwix-tools#205

veloman-yunkan

This will break projects depending on kiwix-serve suggestion endpoint. How many of them do we know about? Don't we have to preserve backward compatibility (for example by adding a new endpoint, say 'suggesturl')? I am just raising those questions to make sure that we didn't overlook anything.

veloman-yunkan · 2021-03-17T11:50:01Z

src/server/internalServer.cpp

@@ -427,7 +427,7 @@ std::unique_ptr<Response> InternalServer::handle_suggest(const RequestContext& r
  if (reader->hasFulltextIndex()) {
    MustacheData result;
    result.set("label", "containing '" + term + "'...");
-    result.set("value", term + " ");
+    result.set("url", "search?content=" + bookName + "&pattern=" + term + "+");


Doesn't term have to be URL-escaped here?

@veloman-yunkan right will do that......I should use kiwix::urlEncode for that purpose right?

If kiwix::parseMimetypeCounter() cannot be utilized for that purpose and/or you can make kiwix::urlEncode() work with minimal tweaks, then yes, go for it.

P.S. 😉

veloman-yunkan · 2021-03-17T11:58:49Z

static/skin/taskbar.js

@@ -20,9 +20,15 @@ function htmlDecode(input) {
              }
        },

+        focus: function(event, ui) {
+            $( "#kiwixsearchbox" ).val(ui.item.label);
+            return false;


My level of competence with jquery and DOM events is almost zero, so can you please explain why you return false here?

by default select and focus try to place ui.item.value in the searchbox but we need to place ui.item.label as our value now contains a URL, even if I write the line $( "#kiwixsearchbox" ).val(ui.item.label); the autocomplete widget will continue with propagating the event ahead and will again change the value to ui.item.value.

From the doc, https://api.jqueryui.com/autocomplete/#option-source
label is the value used in the drop down menu, value is the value inserted in the input field.

It would be better to follow this convention as the suggest endpoint is mainly designed to be used by jqueryui.autocomplete.

We could keep the label/value as it is and add a kind/path/term in the item.
This way we don't need a new focus function, we don't break the api for other user of suggest endpoint and we can correctly handle the item with our js (see other comments)

veloman-yunkan · 2021-03-17T12:01:20Z

static/templates/suggestion.json

@@ -1,7 +1,7 @@
 [
  {{#suggestions}}{{^first}},{{/first}}
  {
-    "value" : "{{value}}",
+    "value" : "{{url}}",


url will be HTML escaped/encoded here (in particular & will be replaced with &). That may seem wrong, but the current design of the full (front-end + back-end) system assumes that value is HTML-decoded in the front-end. Please put a comment here making that explicit.

what you mean here is I should write & in place of & in L430 InternalServer.cpp, other than that what comment should be added?
something like url received should be html encoded

See http://mustache.github.io/mustache.5.html. The {{variable}} syntax applies HTML escaping. For a URL that may seem wrong. The comment must say that this is intentional, since the receiving side in taskbar.js un-escapes this field.

mgautierfr · 2021-03-17T13:29:02Z

You've took the wrong direction with this PR.
The workflow with the current code is:

Do a suggestion search (http://server.com/suggest?content=<zim>&term=<query>).
This return a (json) list of title/value(title) item.
When user clicks on a suggestion, (or any validation kind of validation of the search), we do a search (http://server.com/search?content=<zim>&pattern=<query>)
The search in the backend start to look if there is a article with the exact title (https://github.com/kiwix/kiwix-lib/blob/master/src/server/internalServer.cpp#L497-L519). If yes it redirect to the" final" url (http://server.com/<zim>/<article_path>), if not it return a search page result.

So when the user click on a suggestion, it is redirected to the article.
The problem is that we do extra steps and it is not efficient.

The workflow should be :

Do a suggestion search (http://server.com/suggest?content=<zim>&term=<query>).
This return a (json) list of title/path item.
When user clicks on a suggestion, directly go to http://server.com/<zim>/<path>
If user do a real search, do it (http://server.com/search?content=<zim>&pattern=<query>)
The search in the backend do not try to find a exact match and directly generate the search page.

This will break projects depending on kiwix-serve suggestion endpoint. How many of them do we know about? Don't we have to preserve backward compatibility (for example by adding a new endpoint, say 'suggesturl')? I am just raising those questions to make sure that we didn't overlook anything.

I'm not aware of any projects using it. But @kelson42 or @rgaudin may know some.
If there is no known project I would say that we can change it as we which as it is mainly designed to be internal.
If there is some it would be nice to see with them to not use it, and then start to design a real restfull api projects can use.

static/templates/suggestion.json

veloman-yunkan · 2021-03-18T17:11:56Z

You've took the wrong direction with this PR.
The workflow with the current code is:

Do a suggestion search (http://server.com/suggest?content=<zim>&term=<query>).

This return a (json) list of title/value(title) item.

When user clicks on a suggestion, (or any validation kind of validation of the search), we do a search (http://server.com/search?content=<zim>&pattern=<query>)

The search in the backend start to look if there is a article with the exact title (https://github.com/kiwix/kiwix-lib/blob/master/src/server/internalServer.cpp#L497-L519). If yes it redirect to the" final" url (http://server.com/<zim>/<article_path>), if not it return a search page result.

So when the user click on a suggestion, it is redirected to the article.
The problem is that we do extra steps and it is not efficient.

The workflow should be :

Do a suggestion search (http://server.com/suggest?content=<zim>&term=<query>).

This return a (json) list of title/path item.

When user clicks on a suggestion, directly go to http://server.com/<zim>/<path>

If user do a real search, do it (http://server.com/search?content=<zim>&pattern=<query>)

The search in the backend do not try to find a exact match and directly generate the search page.

@mgautierfr What is in particular wrong about this PR? As far as I understand it is along the lines outlined in your proposed flow - now selecting a suggestion avoids sending a search request and directly goes to the suggested page.

veloman-yunkan · 2021-03-18T18:20:59Z

@MananJethwani Please rebase your branch and squash all commits into one.

MananJethwani · 2021-03-18T18:35:12Z

@veloman-yunkan done

kelson42 · 2021-03-19T08:46:31Z

This will break projects depending on kiwix-serve suggestion endpoint. How many of them do we know about? Don't we have to preserve backward compatibility (for example by adding a new endpoint, say 'suggesturl')? I am just raising those questions to make sure that we didn't overlook anything.

@veloman-yunkan It is true that this will break compatibility. We know about a few of third party software dealing with this API. Considering that this should be easy to adapt their code and that basically how it was working before was buggy, I'm ready to break the backward compatibility.

kelson42 · 2021-03-19T08:52:23Z

@mgautierfr Thx for the cristal clear explanation about how it works and should work!

mgautierfr · 2021-03-30T16:13:02Z

@mgautierfr What is in particular wrong about this PR? As far as I understand it is along the lines outlined in your proposed flow - now selecting a suggestion avoids sending a search request and directly goes to the suggested page.

It is technically avoiding an unnecessary search yes.
But there is some kind of confusion and lack of separation of concern

Here, we have the suggestion backend doing the search but also define what to do with the result (it computes the url to use).

It would be to the frontend to get the raw results and build the corresponding url.

It would avoid to handle this surprising url escaping in the json.
It would allow us to reuse the suggestion api to do something else than auto-completion.
The json would be shorter (it is a small pro I agree)

And even if we decide that the frontend is dumb and simply use what is computed by backend (It would be a valid design, although I prefer not to go this way), we are currently in the middle of the way. The frontend in the PR still have to add the {root} prefix. Why the backend do not generate the correct url directly ?

On top of that, we now have two ways to do the search. One with the classical form (if user don't use completion system) and one with the js changing directly the windows' location. Not a big issue, but then we have one more thing to think about if we do a change.

This PR is making the code more complex instead of simplifying it and reduce the reusability of the api.

MananJethwani · 2021-03-31T18:34:17Z

@mgautierfr so should I just change the response back to label, value but this time keep the value as the article name and generate the URL in the frontend would that be fine? as that would make the PR follow the separation of concern you talk about.

MananJethwani · 2021-03-31T20:49:54Z

@mgautierfr please have a look now

mgautierfr

@mgautierfr so should I just change the response back to label, value but this time keep the value as the article name and generate the URL in the frontend would that be fine? as that would make the PR follow the separation of concern you talk about.

This is the global idea yes. But see my comments in my review.

static/skin/taskbar.js

mgautierfr · 2021-04-01T08:56:36Z

static/skin/taskbar.js

@@ -20,9 +20,15 @@ function htmlDecode(input) {
              }
        },

+        focus: function(event, ui) {
+            $( "#kiwixsearchbox" ).val(ui.item.label);
+            return false;


From the doc, https://api.jqueryui.com/autocomplete/#option-source
label is the value used in the drop down menu, value is the value inserted in the input field.

It would be better to follow this convention as the suggest endpoint is mainly designed to be used by jqueryui.autocomplete.

We could keep the label/value as it is and add a kind/path/term in the item.
This way we don't need a new focus function, we don't break the api for other user of suggest endpoint and we can correctly handle the item with our js (see other comments)

static/skin/taskbar.js

mgautierfr · 2021-04-01T09:07:06Z

static/skin/taskbar.js

+                  item.value = encodeURI(htmlDecode(item.value));
+
+                  if (item.label === `containing '${item.value}'...`) {
+                      item.value = `${root}/search?content=${bookName}&pattern=${item.value}+`;


(This is a open questioning, not a asked change)

The +(space) at the end of the request is to force a real search (avoid a redirection in the backend if the title exact matches https://github.com/kiwix/kiwix-lib/blob/master/src/server/internalServer.cpp#L514-L519).

If we remove the redirection in the backend, we don't need to a space at the end of the query.
As for suggestion, it may break some third party user (I don't know about any).
@kelson42 may have a opinion about this but I think we can remove this redirection in the backend and make the search endpoint always do a search.

veloman-yunkan

The title of this PR must be updated

static/skin/taskbar.js

MananJethwani · 2021-04-01T13:27:39Z

src/server/internalServer.cpp

@@ -428,6 +430,8 @@ std::unique_ptr<Response> InternalServer::handle_suggest(const RequestContext& r
    MustacheData result;
    result.set("label", "containing '" + term + "'...");
    result.set("value", term + " ");
+    result.set("kind", "pattern");
+    result.set("path", "");


needed to add path here as if a path was undefined for some part of suggestion mustache won't render
another way was to remove the path here and add

{{^path}} "path" : "" {{/path}}

in suggestion.json
other than that If I tried to remove the path from this line and not add above-mentioned part in suggestion.json it won't render and errors won't appear

I tried finding if this is how conditioning works in mustache but couldn't find much, this works but might be a bit wrong
@mgautierfr @veloman-yunkan can you help me if this seems wrong

The mustache format is documented here : https://mustache.github.io/mustache.5.html

veloman-yunkan · 2021-04-06T09:18:53Z

static/templates/suggestion.json

+    "kind" : "{{kind}}",
+    {{#path}}
+      "path" : "{{path}}"
+    {{/path}}


For an empty path this will result in an invalid JSON (a comma followed by a closing brace, which confuses the strict parsers).

so what should I do about it?....I confirm for empty path my current solution works fine

Move the comma

thanks I understand what you meant now

veloman-yunkan · 2021-04-06T09:25:47Z

static/skin/taskbar.js

        dataType: "json",
        cache: false,

        response: function( event, ui ) {
-
              for(const item of ui.content) {
                  item.label = htmlDecode(item.label);
                  item.value = htmlDecode(item.value);


Since path is HTML-encoded in suggestion.json (that is important for producing a valid JSON in case if path contains a double quote symbol), it must be htmlDecode-ed here, too.

mgautierfr · 2021-04-07T15:10:46Z

I approve this PR.
Can you rebase on last master before I do the final approval and merge it ?

MananJethwani · 2021-04-07T15:15:20Z

sure

MananJethwani · 2021-04-07T15:23:03Z

@mgautierfr done, should I rebase it as well?

mgautierfr · 2021-04-07T15:25:03Z

@mgautierfr done, should I rebase it as well?

humm... You've just done it no ? What do you want to rebase more ?

MananJethwani · 2021-04-07T15:27:10Z

sorry I meant squash

mgautierfr · 2021-04-07T15:33:15Z

Yes, you may.
There is no real reason to split this change in different commits.

…it in autocomplete

stardiviner · 2021-04-08T01:27:40Z

Is this PR in Docker image now?

kelson42 · 2021-04-08T10:47:47Z

@stardiviner No, we will need a new release of the libkiwix and a new release/build of the kiwix-tools (kiwix-serve) first. This will take a few additional weeks.

stardiviner · 2021-04-08T11:42:22Z

Ok, Thanks for answering. @kelson42

kelson42 requested a review from veloman-yunkan March 17, 2021 06:17

veloman-yunkan requested changes Mar 17, 2021

View reviewed changes

MananJethwani requested a review from veloman-yunkan March 18, 2021 15:22

veloman-yunkan requested changes Mar 18, 2021

View reviewed changes

static/templates/suggestion.json Outdated Show resolved Hide resolved

veloman-yunkan requested a review from mgautierfr March 18, 2021 17:13

veloman-yunkan approved these changes Mar 18, 2021

View reviewed changes

MananJethwani force-pushed the issue/kiwix-tools/205 branch from e97a89a to 246c1be Compare March 18, 2021 18:30

MananJethwani requested a review from veloman-yunkan March 31, 2021 20:49

mgautierfr requested changes Apr 1, 2021

View reviewed changes

veloman-yunkan requested changes Apr 1, 2021

View reviewed changes

static/skin/taskbar.js Outdated Show resolved Hide resolved

MananJethwani requested review from mgautierfr and veloman-yunkan April 1, 2021 13:21

MananJethwani commented Apr 1, 2021

View reviewed changes

MananJethwani changed the title ~~changed suggest response to label,url in place of label,value~~ adding kind and path attributes to suggest response object and using it in autocomplete Apr 3, 2021

MananJethwani force-pushed the issue/kiwix-tools/205 branch from 12f8e8f to 9626b03 Compare April 4, 2021 06:49

veloman-yunkan requested changes Apr 6, 2021

View reviewed changes

MananJethwani force-pushed the issue/kiwix-tools/205 branch from fe25446 to 59e3faf Compare April 6, 2021 14:48

MananJethwani requested a review from veloman-yunkan April 6, 2021 14:53

veloman-yunkan approved these changes Apr 7, 2021

View reviewed changes

MananJethwani force-pushed the issue/kiwix-tools/205 branch from 59e3faf to f690098 Compare April 7, 2021 15:22

adding kind and path attributes to suggest response object and using …

5cb276a

…it in autocomplete

MananJethwani force-pushed the issue/kiwix-tools/205 branch from f690098 to 5cb276a Compare April 7, 2021 15:35

mgautierfr approved these changes Apr 7, 2021

View reviewed changes

mgautierfr merged commit ba44033 into kiwix:master Apr 7, 2021

mgautierfr mentioned this pull request Apr 7, 2021

Remove redirect to article in search #481

Closed

maneeshpm mentioned this pull request Apr 19, 2021

& in the suggestions gets to & in the search field kiwix/kiwix-tools#453

Closed

adding kind and path attributes to suggest response object and using it in autocomplete #464

adding kind and path attributes to suggest response object and using it in autocomplete #464

Conversation

MananJethwani commented Mar 17, 2021

veloman-yunkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgautierfr commented Mar 17, 2021 • edited Loading

veloman-yunkan commented Mar 18, 2021 • edited Loading

veloman-yunkan commented Mar 18, 2021

MananJethwani commented Mar 18, 2021

kelson42 commented Mar 19, 2021

kelson42 commented Mar 19, 2021

mgautierfr commented Mar 30, 2021 • edited Loading

MananJethwani commented Mar 31, 2021 • edited Loading

MananJethwani commented Mar 31, 2021

mgautierfr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

MananJethwani Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgautierfr commented Apr 7, 2021

MananJethwani commented Apr 7, 2021

MananJethwani commented Apr 7, 2021

mgautierfr commented Apr 7, 2021

MananJethwani commented Apr 7, 2021

mgautierfr commented Apr 7, 2021

stardiviner commented Apr 8, 2021

kelson42 commented Apr 8, 2021

stardiviner commented Apr 8, 2021

mgautierfr commented Mar 17, 2021 •

edited

Loading

veloman-yunkan commented Mar 18, 2021 •

edited

Loading

mgautierfr commented Mar 30, 2021 •

edited

Loading

MananJethwani commented Mar 31, 2021 •

edited

Loading

MananJethwani Apr 1, 2021 •

edited

Loading