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

Legion DPI fix #223

Merged
merged 22 commits into from
May 1, 2018
Merged

Conversation

BazookaMusic
Copy link
Contributor

@BazookaMusic BazookaMusic commented Apr 1, 2018

Fix for issue #172,dpi settings properly accounted for.

@BazookaMusic BazookaMusic changed the title Legion fix Legion DPI fix Apr 1, 2018
@LexiconCode LexiconCode added Bug Unexpected behavior from existing features. Testing Required Requesting testers before merge Complete Pull request is complete and tested as defined by Contributor labels Apr 1, 2018
@Versatilus
Copy link
Collaborator

I don't have the hardware to properly test this (but it looks good on the surface), so I can't help with testing it.

@LexiconCode LexiconCode removed the Bug Unexpected behavior from existing features. label Apr 1, 2018
@BazookaMusic
Copy link
Contributor Author

I also do not have multiple monitors however this works for my single Monitor with 150% scaling. You can try adjusting the display scaling slider yourself to see if this works. The boxes should be properly placed.

horizontal_dpi = windll.gdi32.GetDeviceCaps(
dc, LOGPIXELSX) # Horizontal DPI determines the DPI scaling value
windll.user32.ReleaseDC(0, dc)
dpi_scaling = dpi_map[horizontal_dpi]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was just thinking about ways I could test this and that made me wonder if there is any software that could fake it on my current hardware. In order to keep the simulated resolution high enough to be usable it wouldn't necessarily scale at the same rates horizontally and vertically. It could also have weird DPI values.

What if a piece of hardware doesn't have square pixels? Also, wouldn't it be easier/faster to just divide the given DPI value by 96.0?

@chilimangoes
Copy link
Collaborator

I have multiple monitors and will give this a try at work tomorrow. What I really wonder is whether this works on really high DPI screens, like they have on some laptops. I don't have anything like that to test with.

@LexiconCode
Copy link
Member

I have access to a 4K screen and will test tomorrow afternoon

@LexiconCode
Copy link
Member

LexiconCode commented Apr 2, 2018

I speak the Legion command and nothing happens. It looks like Legiongrid is not executed

Little bit of debugging
Traceback (most recent call last): File "c:\NatLink\NatLink\MacroSystem\caster\asynch\mouse\legion.py", line 252, in <module> main(sys.argv[1:]) File "c:\NatLink\NatLink\MacroSystem\caster\asynch\mouse\legion.py", line 219, in main dpi_scaling = dpi_scaling/96.0 UnboundLocalError: local variable 'dpi_scaling' referenced before assignment PS C:\NatLink\NatLink\MacroSystem>

@BazookaMusic
Copy link
Contributor Author

There's a typo from autocomplete in the last change. I won't be home for about an hour to change it. I have added a comment in the code if you want to test it before I can change it.

@LexiconCode
Copy link
Member

LexiconCode commented Apr 2, 2018

legion at 250% scale
3840 x 2160 screen resolution
Duplicated Display result is the same as the picture above
250% scale

@chilimangoes
Copy link
Collaborator

It looks like the problem is exacerbated on multiple monitors. At work I have a 1280x1024 and a pair of 1920x1080 monitors. If I set them all to 150%, and say "legion 3" (the rightmost monitor, labeled 2 in the screenshot), the legion grid is both too small and overlaps with monitor 1 in the middle. The good news is the legion rectangles are in the right places over the text. It looks like it's just a problem with the calculation of the screen capture rectangle and subsequent positioning of the Legion Grid window. Almost makes me wonder if we need to calculate anything or if just setting DPI awareness would be enough.

legion_dpi

@BazookaMusic
Copy link
Contributor Author

BazookaMusic commented Apr 2, 2018

@LexiconCode try signing out and back in, windows fails to update the dpi value without a reboot or sign out. If it works I will try to find a more reliable way to get the settings.
@chilimangoes Just setting awareness results in proper positioning but a smaller rectangle for me.

@BazookaMusic
Copy link
Contributor Author

BazookaMusic commented Apr 3, 2018

I added the scaling to the x and y values as well. I think this Could fix @chilimangoes' problem of the screen overlapping. Do try to sign out and back in,after setting the scaling ,however else windows reports the previous dpi settings rendering the solution useless.

@LexiconCode
Copy link
Member

LexiconCode commented Apr 3, 2018

screenshot_1

capture

Strange screenshots I've tried signing out and rebooting. Happens with snip tool and other programs.

@BazookaMusic
Copy link
Contributor Author

Is it just the screenshots or is it still completely whack?

@LexiconCode
Copy link
Member

Just the screenshots

@chilimangoes
Copy link
Collaborator

chilimangoes commented Apr 3, 2018

That completely fixed the problem as long as all three monitors have the same DPI scaling. However, when I set monitor 1 to 100% scaling and the other two monitors to 125%, the "Legion" command works fine on monitor one, but looks like this on the other two monitors. My guess is the problem is that the DC you're grabbing (NULL) is for the entire desktop. In order for the scaling to work properly, I imagine you'll need to get the DC for each monitor and calculate them separately.

untitled

@LexiconCode LexiconCode added Dragonfly A catchall label for issues related to Dragonfly Bug Unexpected behavior from existing features. and removed Testing Required Requesting testers before merge WIP An work in progress labels Apr 6, 2018
@chilimangoes
Copy link
Collaborator

Note that fixing dragonfly would be more involved because it currently gets the monitor information on boot up and caches it in the exported monitors variable. But we want to retrieve fresh monitor configuration info each time so the user doesn't have to reboot dragon any time they change their DPI settings.

@LexiconCode
Copy link
Member

We might hold off posting the issue in Danesprite's repository until I've had a chance to discuss this with him.

@BazookaMusic
Copy link
Contributor Author

We could always make the monitors variable a class with getters which would fetch the info on runtime. Or even add a new class.For Legion however to make the grid dpi aware an operating System check will be needed anyway. I don't think It's possible to make it completely cross-platform without dealing with each operating system's annoyances.

@chilimangoes
Copy link
Collaborator

That's a good idea. My Python is a bit rusty and it didn't even occur to me to use a class with an indexer property, or whatever Python calls them.

@Versatilus
Copy link
Collaborator

I'll admit that the source code for it is available and public domain, but since we're talking about using a Windows DLL that isn't currently implemented on other platforms, isn't the whole discussion kind of academic at this point?

Assuming this code is working for everyone and most people are satisfied with the box scaling code(though honestly I think it should be a setting in the configuration file), I think we should merge this as it is and separately work with Dane on fleshing out the cross-platform aspects of Dragonfly to better support this.

@chilimangoes
Copy link
Collaborator

Absolutely. My thought was to just give it another couple days to let the dust settle on these latest commits to see if there are any other issues before pulling the merge trigger.

@Versatilus
Copy link
Collaborator

Versatilus commented Apr 6, 2018 via email

@BazookaMusic
Copy link
Contributor Author

Don't pull yet, I will put the option in the settings.

@LexiconCode
Copy link
Member

Excellent, I'm a little like that with trigger-happy as well.

@LexiconCode
Copy link
Member

LexiconCode commented Apr 8, 2018

@gerrish Danesprite is fine with issues and enhancements related to dragonfly being posted in his fork.

@BazookaMusic
Copy link
Contributor Author

I have restored the dependency on dragonfly and tested it with danesprite's update. It works well and should make it easier to implement a multiplatform solution in the future.

@LexiconCode
Copy link
Member

LexiconCode commented Apr 13, 2018

A note for documentation.
The DPI fix implementation is not cross-platform in dragonfly - Windows only. Status of general dragonfly cross-platform implementation.

@LexiconCode LexiconCode added the Documentation Issues related to Documentation label Apr 13, 2018
@BazookaMusic
Copy link
Contributor Author

What is required to finally merge this pull request? Something like changing the way caster is installed to include the modified dragonfly?

@LexiconCode
Copy link
Member

LexiconCode commented Apr 16, 2018

@gerrish we need to update the documentation to include Danesprite/dragonfly fork.

Readme
readthedocs

Optionally update to include Danesprite/dragonfly dependencies
dragonfly_upgrade.bat
pip.bat

@LexiconCode LexiconCode added this to the v6 milestone Apr 16, 2018
@BazookaMusic
Copy link
Contributor Author

Since the documentation currently uses videos to explain the installation process, how do you suggest we include the fork? Simply as a mention with a note to follow the instructions in the git hub page or a complete remake with full explanation?

@LexiconCode
Copy link
Member

LexiconCode commented Apr 19, 2018

It's up to you although I'm leaning towards keeping it simple by including a note. The install procedure may change if we are able to use https://github.com/synkarius/caster/issues/217.

@BazookaMusic
Copy link
Contributor Author

It's done!

@LexiconCode LexiconCode merged commit 5694c41 into dictation-toolbox:develop May 1, 2018
@BazookaMusic BazookaMusic deleted the Legion_fix branch May 3, 2018 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Unexpected behavior from existing features. Complete Pull request is complete and tested as defined by Contributor Documentation Issues related to Documentation Dragonfly A catchall label for issues related to Dragonfly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants