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

Qt/GameList: Also filter by filename when searching. #11583

Merged

Conversation

danielgarzaf
Copy link
Contributor

@Pokechu22
Copy link
Contributor

Code looks good to me (though I haven't tested it out). You do have a lint/formatting issue, though; see https://dolphin.ci/#/builders/10/builds/10374 for the fix (or if you're using Visual Studio, press Control+K Control+D to format the file).

@danielgarzaf
Copy link
Contributor Author

I figured I could also add a condition to check if the File Name column is not hidden (it's hidden by default). Please let me know if that's okay

@Pokechu22
Copy link
Contributor

That seems reasonable to me, though I think a similar check on the title column would be a bad idea.

!(QString::fromStdString(game.GetFileName()).contains(m_term, Qt::CaseInsensitive) &&
show_filename))
Copy link
Contributor

Choose a reason for hiding this comment

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

show_filename && string.contains(m_term) would be better due to short-circuit evaluation; checking whether one string contains another string is a slower operation that it would be better to skip if the filename column is hidden. In this case it probably doesn't matter that much (even if someone has all of the GameCube and Wii discs, that's only about 5000 of them, which probably could still be handled in milliseconds), but it's something to keep in mind for other situations.

This logic is also getting a bit messy, though; maybe something like this would be clearer? (I haven't tested this code and am not sure if it actually compiles/is formatted correctly)

if (!m_term.isEmpty())
{
  const bool matches_title = !QString::fromStdString(game.GetName(m_title_database)).contains(m_term, Qt::CaseInsensitive));
  const bool filename_visible = Config::Get(Config::MAIN_GAMELIST_COLUMN_FILE_NAME);
  const bool matches_filename = filename_visible && !(QString::fromStdString(game.GetFileName()).contains(m_term, Qt::CaseInsensitive);
  if (!(matches_title || matches_filename))
  {
    return false;
  }
}

@danielgarzaf
Copy link
Contributor Author

Done. I had to remove the negated logic on both string searches. Thanks for the suggestion!
I could also add a Qt::CaseSensitivity cs variable to help a bit with the formatting, but it's just style at this point

@Pokechu22
Copy link
Contributor

Code looks good to me.

I noticed two odd behaviors, though:

  • Turning the file name on or off while a search is already entered doesn't update results.
  • The file name column itself isn't visible on the grid view, but the setting for whether the column is enabled still affects things.

Neither of these are really worth worrying about, though. The column setting can still be toggled on the grid view from the view menu, so even if the columns aren't visible there it's still adjustable.

@danielgarzaf
Copy link
Contributor Author

Nice catch on the grid view bug. I'll push the fix for that in a bit.

Regarding the first point, I'm not sure how to fix it at the moment. This is my first time with Qt, so I think I have to learn a bit more about it to figure out a simple solution.

But as you said, it's not that important.

@AdmiralCurtiss AdmiralCurtiss changed the title Seach feature now finds matches by filename too Qt/GameList: Also filter by filename when searching. Feb 26, 2023
@AdmiralCurtiss AdmiralCurtiss merged commit 4fb3042 into dolphin-emu:master Feb 26, 2023
14 checks passed
@danielgarzaf danielgarzaf deleted the seach-results-filename branch February 27, 2023 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants