Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

as-cache: Use correct FTS value per result #268

Merged
merged 1 commit into from Apr 22, 2020

Conversation

davidmhewitt
Copy link
Contributor

I've been looking into some of the searching/sorting logic and I'm not 100% sure on this change due to not being overly familiar with the code and not having worked the logic all the way back yet, but given that

ENTRY_LEN = sizeof(AsTokenType) + AS_CACHE_CHECKSUM_LEN * sizeof(guint8);

It looks like there is one AsTokenType per result, yet it seems the pointer arithmetic for updating sort_score doesn't take the array index into account.

@ximion
Copy link
Owner

ximion commented Apr 22, 2020

Oh wow... I think you are correct with your assumptions, the current code will always read the match score of the search token for the first component in the list, and not of the one that we are actually looking for, which yields worse search results.
Potentially adding a test for search sorting makes sense, so we can spot unwanted sorting order changes in the future.

Do you have a real-world example that this change made better? (I'm curious as to how you found this in the first place)

In any case, the patch as it is looks fine to me, thank you very much!

@ximion ximion merged commit 782793d into ximion:master Apr 22, 2020
1 check passed
@davidmhewitt
Copy link
Contributor Author

davidmhewitt commented Apr 22, 2020

I came across this by chance when I was trying to figure out where the score actually came from so I could write #269 , which I'd greatly appreciate your input on when you have time 🙂

I tested this with en_GB localisation on elementary OS (technically Ubuntu 18.04 AppStream data with a few extras) with a search query of "builder", and the results were definitely better.

Before:

Identifier: org.gnome.Devhelp.desktop [desktop-application]
Name: Devhelp
Summary: A developer tool for browsing and searching API documentation
Package: devhelp
Homepage: https://wiki.gnome.org/Apps/Devhelp/
Icon: devhelp_devhelp.png
---
Identifier: avogadro.desktop [desktop-application]
Name: Avogadro
Summary: Molecular Graphics and Modelling System
Package: avogadro
Icon: avogadro_avogadro-icon.png
---
Identifier: fwbuilder.desktop [desktop-application]
Name: Firewall Builder
Summary: Firewall administration tool GUI
Package: fwbuilder
Icon: fwbuilder_firewall_64.png
---
Identifier: org.gnome.Builder.desktop [desktop-application]
Name: Builder
Summary: An IDE for GNOME
Package: gnome-builder
Homepage: https://wiki.gnome.org/Apps/Builder
Icon: gnome-builder_org.gnome.Builder.png
---
Identifier: glade.desktop [desktop-application]
Name: Glade
Summary: Create or open user interface designs for GTK+ applications
Package: glade
Homepage: http://glade.gnome.org/
Icon: glade_glade.png
---
Identifier: eclipse.desktop [desktop-application]
Name: Eclipse
Summary: Eclipse Integrated Development Environment
Package: eclipse-platform
Icon: eclipse-platform_eclipse.png

After:

Identifier: fwbuilder.desktop [desktop-application]
Name: Firewall Builder
Summary: Firewall administration tool GUI
Package: fwbuilder
Icon: fwbuilder_firewall_64.png
---
Identifier: org.gnome.Builder.desktop [desktop-application]
Name: Builder
Summary: An IDE for GNOME
Package: gnome-builder
Homepage: https://wiki.gnome.org/Apps/Builder
Icon: gnome-builder_org.gnome.Builder.png
---
Identifier: glade.desktop [desktop-application]
Name: Glade
Summary: Create or open user interface designs for GTK+ applications
Package: glade
Homepage: http://glade.gnome.org/
Icon: glade_glade.png
---
Identifier: org.gnome.Devhelp.desktop [desktop-application]
Name: Devhelp
Summary: A developer tool for browsing and searching API documentation
Package: devhelp
Homepage: https://wiki.gnome.org/Apps/Devhelp/
Icon: devhelp_devhelp.png
---
Identifier: avogadro.desktop [desktop-application]
Name: Avogadro
Summary: Molecular Graphics and Modelling System
Package: avogadro
Icon: avogadro_avogadro-icon.png
---
Identifier: eclipse.desktop [desktop-application]
Name: Eclipse
Summary: Eclipse Integrated Development Environment
Package: eclipse-platform
Icon: eclipse-platform_eclipse.png

Edit: worth noting that we've already backported 0.12.10 to this version of elementary, so there probably weren't any other search changes that would have influenced the above output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants