Refactor gui_templates.h
#7159
Refactor gui_templates.h
#7159
Conversation
|
Looks good to me! 👍 Concerning the new home for the functionality: is there some kind of GUI helpers file where it could move? |
| } | ||
|
|
||
| // to calculate DPI of a screen to make it HiDPI ready | ||
| qreal devicePixelRatio = QGuiApplication::primaryScreen()->devicePixelRatio(); |
There was a problem hiding this comment.
When using QGuiApplication::primaryScreen() i think there could be some issues when Switching the App to an other Monitor with an diffrent DPI, We should grab the current Monitor for our Window
There was a problem hiding this comment.
I get what you mean but can you suggest a way to handle this one properly?
There was a problem hiding this comment.
I think this is something that should be handled by the GUI framework, i.e. by Qt. It just one example of why using these functions in application code is problematic.
Using code like this also won't do the "right thing" when LMMS is in the middle of being dragged from one screen to another with different DPI values. Qt likely knows that one part of the application rectangle must be rendered with different settings than the other and can do so. The application code does not even have the knowledge of the current state ("I am shown on two different monitors.") and cannot do the right thing.
Looking at the sources, i didn't really find a more suitable file. |
If it only contains this single function then another option might be to rename the file to |
|
I'll hold off on the rename for now, to reduce the diff. |
|
I am thinking of moving this to the gui namespace but i am doubtful about the changes I need to make in plugin code. + I have some other minor changes in mind which I'll test soon. |
Such a move would definitively make sense. The code should only be used from GUI code and not from core code anyway. |
|
This pr got 2 approvals but it ain't ready for merge yet. I got a few more minor cleanups to do. So can you wait for an extra day. |
Mark it as a draft and it will prevent merge 🙂 |
|
I'm done with my changes, let the ci run before i make this pr ready for review. |
I started off trying to remove the
QApplication::desktopcall per #6614 but I ended up doing a refactor.I removed the duplicate
pointSizeFfunction and merged it into thepointSizefunction.There's a chance that this PR might improve HiDPI support as I did some changes in that area but haven't tested.
Todo: delete the file and find a new home for the pointSize function (should we really keep a file for a single function?)
inviting @michaelgregorius for review