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

#24626 misc palette search issues #24647

Merged

Conversation

krasko78
Copy link
Contributor

@krasko78 krasko78 commented Sep 11, 2024

Resolves: #24626
Resolves: #15449

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

This commit makes MSS remember the last search results so when the user activates the search again, the search results properly reflect the search string.
… displayed and CTRL+F9 is pressed to start a search
@krasko78 krasko78 force-pushed the #24626-MiscPaletteSearchIssues branch from 08747c3 to cea42ce Compare September 14, 2024 22:49
…ged parent of palette context menus,, using ContextMenuLoader
@krasko78 krasko78 force-pushed the #24626-MiscPaletteSearchIssues branch from cea42ce to fe35472 Compare September 16, 2024 09:42
@zacjansheski
Copy link
Contributor

I can produce a crash now (perhaps this should be fixed later on)

  1. In text palette, select more
  2. Command + F9, search for "ba"
  3. esc
  4. select more again
  5. Command + F9, clear search and search for "ba" again
video1675711105.mp4

@zacjansheski
Copy link
Contributor

PaletteSearchCrash.zip

@krasko78
Copy link
Contributor Author

Crash confirmed. Thanks @zacjansheski! I'll look into it.

@krasko78
Copy link
Contributor Author

I think I fixed it.

…sible and an undefined assignment error to the expanded property
@krasko78 krasko78 force-pushed the #24626-MiscPaletteSearchIssues branch from c0a989b to ac99311 Compare September 16, 2024 23:14
@zacjansheski
Copy link
Contributor

Tested on MacOS 14, Windows 11, Ubuntu 22.04.3. Approved
#24626 FIXED
#15449 FIXED

@krasko78
Copy link
Contributor Author

@cbjeukendrup If I make changes after the PR has been approved (like in this case a crash was discovered during the testing), how are you guys supposed to review them? Should I notify you, re-request a review or something?

@cbjeukendrup
Copy link
Contributor

We're usually already notified by GitHub's email notifications like "@\krasko pushed 1 new commit", so we take one more look before merging it.
@RomanPudashkin Would you mind taking care of doing that for this PR?

@RomanPudashkin RomanPudashkin merged commit 5c00dd7 into musescore:master Sep 18, 2024
11 checks passed
@krasko78 krasko78 deleted the #24626-MiscPaletteSearchIssues branch October 22, 2024 18:53
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