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

RFC: Use custom matcher for games list #3076

Merged
merged 3 commits into from Nov 1, 2021

Conversation

@lolbot-iichan
Copy link
Contributor

@lolbot-iichan lolbot-iichan commented Jun 19, 2021

This PR extends filter of games list to use some simple google-like syntax.

Filter is split into tokens, each token are checked separately, if any token does not match a game, it's filtered out.

Oldfashioned requests are still supported with tokens without ":", "=", "~":

  • Hamlet Hungarian - games that have "Hamlet" and "Hungarian" substrings in description

Three kinds of property checkers added:

  • "=" is used to check exact value of something, e.g. gameid=reversion2 - games with exact gameid match
  • ":" is used to search substring at something, e.g. description:: - games with ":" substring at description
  • "~" is used to match wildcard against something, e.g. path:D:\\games\\* - games at Windows folder D:\Games\

6 properties can be abbrivated:

  • For "description" you can use "d", "de", ... "description"
  • For "engineid" you can use "e", "en", ... "engineid"
  • For "gameid" you can use "g", "ga", ... "gameid"
  • For "language" you can use "l", "la", ... "language"
  • For "path" you can use "p", "pa", "pat", "path"
  • For "platform" you can use "pl", ... "platform"

Any other property can be checked as well:

  • guioptions:noLang - games with "noLang" option at "guioptions" property of ConfMan
  • extra~?* - games with non-empty "extra" property of ConfMan

"!" inverts token result:

  • !GOG - games that don't contain "GOG" substring in description
  • !lang=ru - games not in Russian languange
  • !p:demo - games that don't have "demo" substring in game path
  • !engine~sword# - games not made with sword1 and sword2 engines

Let's try it alltogether:

  • engine=ags path:steamapps !extra:Steam - AGS games at /SteamApps/ folder not marked as Steam game at "extra"
  • e=wintermute l= - Wintermute games with empty "language" property
  • pl:dos lang=he desc~a* - Hebrew games for DOS with description starting with letter "A"

Current grammar is something like this;

FILTER = TOKEN [ WHITESPACE TOKEN ]*
TOKEN = "!"* ( SUBSTRING | GAMEPROPERTY )
GAMEPROPERTY = PROPERTY ( ":" | "=" | "~" ) FILTER
SUBSTRING = string of characters without ":", "=", "~", does not starts with "!"
PROPERTY = string of characters without ":", "=", "~", does not starts with "!"
FILTER = string of any characters

Obligatory screenshot:
изображение

@lolbot-iichan lolbot-iichan changed the title GUI: Use custom matcher for games list (RFC) RFC: Use custom matcher for games list Jun 20, 2021
@lolbot-iichan lolbot-iichan requested a review from sev- Jun 23, 2021
@sev-
Copy link
Member

@sev- sev- commented Jun 24, 2021

This is an excellent improvement. Would you also be keen to put the documentation changes into this PR? https://wiki.scummvm.org/index.php?title=Developer_Central#Contribution_guide_for_technical_writers

Loading

@bluegr
Copy link
Member

@bluegr bluegr commented Jun 24, 2021

Great functionality! Thanks for working on this! +1 from me

Loading

@lolbot-iichan
Copy link
Contributor Author

@lolbot-iichan lolbot-iichan commented Jun 25, 2021

@sev- Added some DOCS to this PR.
I'm not a native English speaker though, so it needs some reviewing.

Loading

Copy link
Contributor

@Thunderforge Thunderforge left a comment

Made a couple of suggestions for this.

Loading


If you have provided several search patterns, only games that match all of them are displayed.

The matches are independent and not ordered, which means that when you are looking for ``Monkey Island``, you would get all the games with words "Monkey" and "Island" in description. Note that imaginary titles like "My Island with some Monkeys" would also be displayed.
Copy link
Contributor

@Thunderforge Thunderforge Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that imaginary titles like "My Island with some Monkeys" would also be displayed.

I don't understand what this means. If there is no game by that name in your library, why would it be displayed?

Loading

Copy link
Contributor Author

@lolbot-iichan lolbot-iichan Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to somehow emphasis the fact that word order does not matter, but I couldn't think of a good example of existing game with Island going before Monkey.

I think of changing it to something like this:

... when you are looking for Open Quest, you would get all the games with words "Open" and "Quest" in description. The results would contain games like "Open Quest (Windows/English)" and "Police Quest IV: Open Season (DOS/Demo)"

Loading


.. note::

The ``platform`` key can't be abbrivated to ``p``, since ``p`` is already used for ``path``.
Copy link
Contributor

@Thunderforge Thunderforge Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if instead we made it f for file, filepath, filesystem, or filesystem-path, thus allowing us to use p for platform?

Loading

Copy link
Contributor Author

@lolbot-iichan lolbot-iichan Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea, but that conflicts the point of having all configuration keys mapped to aptual keys at scummvm.ini file.
So, path would be supported anyway (since there is such key in scummvm.ini) and it may be strange abbrivating path to f.

Also, from the list "f for file, filepath, filesystem, or filesystem-path" I suspect that my idea of abbrivating was actually lost inside translation. Current code allows you to type ANY number of characters of abbrivated configuration key. For language I suggest l, lang and language at Examples column, but you can also use la:en or langu:en or languag:en, etc. Keys filepath and filesystem don't follow this pattern - they have different suffixes. Should I somehow explain this pattern in more details in documentation?

Loading


If search pattern does not contain ``:``/``=``/``~`` characters and it does not start with ``!`` character, then it's used for a case insensitive substring search.

For example, if you type ``m`` you would get all the games containing letter 'M' in description.
Copy link
Contributor

@Thunderforge Thunderforge Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps to reiterate that this is case insensitive, it should say

containing an 'M' or 'm' in the description.

Loading

Copy link
Contributor Author

@lolbot-iichan lolbot-iichan Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll update it to this text;

For example, if you type m you would get all the games containing an 'M' or 'm' in description.

Loading


The filter is applied as you type, there is no need to press Enter or click anything to perform the search.

All searchs are case insensitive, there is no way to force the case sensitive search.
Copy link
Contributor

@Thunderforge Thunderforge Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a technical limitation? It seems like the ability to optionally have a case sensitive search would be a nice feature to have.

Loading

Copy link
Contributor Author

@lolbot-iichan lolbot-iichan Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a limitation of current implementation that may be changed in future.

Should I say All searchs are case insensitive, case sensitive search is not currently implemented. ?
Should I just drop this moment by writing All searchs are case insensitive. ?

Loading


All searchs are case insensitive, there is no way to force the case sensitive search.

To reset the filter and get the full list of games, you could click on a cross icon on the right.
Copy link
Contributor

@Thunderforge Thunderforge Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does ScummVM support right-to-left languages and, if so, would this icon still be on the right for those or would it be on the left?

Loading

Copy link
Contributor Author

@lolbot-iichan lolbot-iichan Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point. ScummVM Launcher indeed support RTL mode for Hebrew, and cross goes to the left in this case.
Also on detailed look it's a button with "X" text, not an actual icon.

Is it ok to checke it to something like this?

To reset the filter and get the full list of games, you could click on a cross located near the text input field.

Loading

Copy link
Member

@sev- sev- Jul 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, your suggestion makes sense to me

Loading


If you have provided several search patterns, only games that match all of them are displayed.

The matches are independent and not ordered, which means that when you are looking for ``Monkey Island``, you would get all the games with words "Monkey" and "Island" in description. Note that imaginary titles like "My Island with some Monkeys" would also be displayed.
Copy link
Contributor

@Thunderforge Thunderforge Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The matches are independent and not ordered

I think this would be confusing, since without searching, they would be alphabetical. If I have:

  • King's Quest II: Romancing the Throne
  • King's Quest III: To Heir is Human
  • King's Quest IV: The Perils of Rosella

And then I search for King's Quest, it would be very surprising if they came back as

  • King's Quest III: To Heir is Human
  • King's Quest II: Romancing the Throne
  • King's Quest IV: The Perils of Rosella

I think it would be better to keep them ordered.

Loading

Copy link
Contributor Author

@lolbot-iichan lolbot-iichan Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not what I intended to say.
The idea was to somehow emphasis the fact that while searching for King's Quest, you are searching for lines that have King's and have Quest, but Quest doesn't have to follow King's. This filter would also match strings like "Space Quest XXIII: Pirate King's New Throne" (there is no such game, of course).

Could you please help me to fix this sentance? Is it something like this?

The result is the same for any search pattern order, when you are looking for Open Quest, you would get all the games with words "Open" and "Quest" in description. The results would contain games like "Open Quest (Windows/English)", but would also contains games like "Police Quest IV: Open Season (DOS/Demo)", with "Quest" search pattern matched before "Open" search pattern.

Loading


- ``p~D:*``
- ``path~D:*``"
``path``, Filesystem path for the game,"
Copy link
Contributor Author

@lolbot-iichan lolbot-iichan Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG, that's a copypaste.
It should say

platform, internal ID of the platform,"

Loading

@@ -56,6 +56,8 @@ class ListWidget : public EditableWidget {
typedef Common::Array<Common::U32String> U32StringArray;

typedef Common::Array<ThemeEngine::FontColor> ColorList;

typedef bool (*FilterMatcher)(void *arg, int idx, const Common::U32String &item, Common::U32String token);
Copy link
Contributor

@mgerhardy mgerhardy Jul 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make sense to pass the token here as reference, too?

Loading

@@ -56,6 +56,7 @@ class LauncherDialog : public Dialog {
void handleKeyUp(Common::KeyState state) override;
void handleOtherEvent(const Common::Event &evt) override;
bool doGameDetection(const Common::String &path);
Common::String getGameConfig(int item, Common::String key);
Copy link
Contributor

@mgerhardy mgerhardy Jul 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't key get passed by ref?

Loading

@sev-
Copy link
Member

@sev- sev- commented Nov 1, 2021

Merging this

Loading

@sev- sev- merged commit 6aa05f4 into scummvm:master Nov 1, 2021
1 check passed
Loading
@sev-
Copy link
Member

@sev- sev- commented Nov 1, 2021

And I manually implemented all the suggested documentation changes

Loading

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