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

Ready for review - [Mouse Jump] Customisable appearance - borders, margins, colours, etc (🎬 Take 2) #32838

Merged

Conversation

mikeclayton
Copy link
Contributor

@mikeclayton mikeclayton commented May 12, 2024

Summary of the Pull Request

This is a re-visit of cancelled PR #29292 / issue #27511 to add customisable styles (2d / 3d effect borders, colours, padding, margins etc) around the Mouse Jump preview screen and the individual screenshots on it.

In the previous PR I hit a dead-end with the logic to upgrade existing Mouse Jump config files and with exposing all the style settings in the Settings UI, so in this PR I've just synched the latest rendering code from my FancyMouse utility but made no attempt to add any new settings to Mouse Jump right now.

As a result, there's no new Settings UI properties or application functionality in this PR - I've left the default Mouse Jump preview style as-is (i.e. flat preview border and no margins or bezels around screens), but this PR will make it easier to expose the style settings in follow-up work if all the rendering code is already there.

(See #27511 for a description of the eventual "customisable preview styles" feature proposal this PR is a step toward)

PR Checklist

  • Closes: - partial implementation of [Mouse Jump] Customisable appearance - borders, margins, colours, etc #27511
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
    • spoke briefly with @crutkas - there's higher priority work right now, but posting this PR so it can be picked up when there's capacity...
  • Tests: Added/updated and all pass
    • new tests from FancyMouse ported into Mouse Jump and all pass
  • Localization: All end user facing strings can be localized
    • no changes required
  • Dev docs: Added/updated
    • no changes required
  • New binaries: Added on the required places
    • no changes required
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx
    • no changes required

Detailed Description of the Pull Request / Additional comments

Mostly a maintenance release to sync "customisable style rendering" code from FancyMouse to Mouse Jump::

  • moved all code shared with FancyMouse into a "Common" folder to make it easier to keep in sync
  • synced customisable style-based preview rendering from FancyMouse to Mouse Jump
  • fixed a bug where number pad keys didn't jump the mouse to the numbered screen
  • unit tests can now test real rendering of the preview image by "capturing" areas of a static test bitmap instead of the live desktop screen and comparing it to an embedded resource containing the expected image, which is a last line of defence against regression bugs

Validation Steps Performed

The latest commit seems to work fine locally, but I've not performed a full regression test yet...

  • Workflow tests
    • Automated tests passing locally
    • Minimal actions workflow (spelling check) passing for PR
    • Full actions workflow (msbuild) passing for PR
  • UI tests
  • Lifecycle tests
    • Starting PowerToys Runner launches MouseJumpUI.exe when enabled, and not when disabled
    • Enabling / disabling Mouse Jump in settings starts / stops MouseJumpUI.exe
    • Exiting PowerToys Runner stops MouseJumpUI.exe
    • Killing runner exe via Task Manager stops MouseJumpUI.exe
    • Stopping Visual Studio local debug run stops MouseJumpUI.exe
      • note - runner needs to be in non-admin mode otherwise Visual Studio debugger disconnects at launch
    • Hotkey and size settings are automatically reloaded when config file is modified from Settings UI
    • Hotkey and size settings are automatically reloaded when config file is modified manually (e.g. in notepad) while runner and MouseJumpUI.exe are running
  • Internal Test Suite
    • Enable Mouse Jump. Then:
    • Press the activation shortcut and verify the screens preview appears.
    • Change activation shortcut and verify that new shortcut triggers Mouse Jump.
    • Click around the screen preview and ensure that mouse cursor jumped to clicked location.
    • Reorder screens in Display settings and confirm that Mouse Jump reflects the change and still works correctly.
    • Change scaling of screens and confirm that Mouse Jump still works correctly.
    • Unplug additional monitors and confirm that Mouse Jump still works correctly.
    • Disable Mouse Jump and verify that the module is not activated when you press the activation shortcut.

@mikeclayton mikeclayton changed the title Dev/mikeclayton/mousejump styles [Mouse Jump] Customisable appearance - borders, margins, colours, etc (🎬 Take 2) May 12, 2024
@mikeclayton mikeclayton changed the title [Mouse Jump] Customisable appearance - borders, margins, colours, etc (🎬 Take 2) Ready for review - [Mouse Jump] Customisable appearance - borders, margins, colours, etc (🎬 Take 2) Jun 8, 2024
Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you for the contribution!
Just had a quick question about a code change.
@mikeclayton , so the idea is to merge this one so another PR can come later with some Settings changes?

@@ -52,7 +52,7 @@ private static IntPtr ToPtr(INPUT[] values)
var size = INPUT.Size;
foreach (var value in values)
{
Marshal.StructureToPtr(value, ptr, true);
Marshal.StructureToPtr(value, ptr, false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why aren't we deleting the old one now? Not sure why this change was made.

Copy link
Contributor Author

@mikeclayton mikeclayton Jun 12, 2024

Choose a reason for hiding this comment

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

The memory is only allocated a few lines earlier so I figured there was actually nothing to delete and it should have been false all along.

Happy to revert it if preferred though?

(Nice spot as well, in what was a pretty sprawling PR :-))

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's what you're doing in other spots you use this as well 🤔 So I assume it must be correct, we didn't have reports of leaks. Although, I wonder. It it was incorrect, why didn't the free cause a crash? It might be correct to clean all those uses instead, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can take a look at the other instances - I must have just spotted this one while making other changes.

Looking at the documentation it says false is the correct value for this scenario:

https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.marshal.structuretoptr?view=net-8.0

On the first call to the StructureToPtr method after a memory block has been allocated, fDeleteOld must be false, because there are no contents to clear.

I'll take a look at the other uses of StructureToPtr and fix any others that need it, if you want to hold off on merging this PR for now...

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's the only place where you used true, I've checked. Good to merge, then. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

awesome. thanks for the thorough review :-)

reading the small print in the documentation i think false might only cause a problem if the structures being marshalled have fields that contain object references, because the runtime keeps a reference count on thosee objects until the fDeleteOld flag is set to true or DestroyStructure is called

If you use the StructureToPtr method to copy a different instance to the memory block at a later time, specify true for fDeleteOld to remove reference counts for reference types in the previous instance. Otherwise, the managed reference types and unmanaged copies are effectively leaked.

We (I) possibly got lucky because all the fields on the marshalled structure are (eventually) value types. That's just speculation though and it's definitely better to comply with the documentation :-)

@mikeclayton
Copy link
Contributor Author

mikeclayton commented Jun 12, 2024

so the idea is to merge this one so another PR can come later with some Settings changes?

@jaimecbernardo - That's correct. I got a bit overwhelmed in the previous (cancelled) PR with the amount of changes needed to pull in the new style rendering code and expose the config options in the Settings UI so I figured it would be more manageable to break it into two steps.

@jaimecbernardo jaimecbernardo merged commit 651f2e4 into microsoft:main Jun 12, 2024
10 checks passed
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.

2 participants