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

Waveform: fix shifted pixmap markers with odd scale factors #3938

Merged
merged 1 commit into from
Jun 4, 2021

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jun 3, 2021

follow-up for #3936 to fix
https://bugs.launchpad.net/mixxx/+bug/1917253

Now the pixmap waveform markers in Deere are

  • correctly scaled
  • in the correct position

For testing set an non-int scale factor like
export QT_SCREEN_SCALE_FACTORS=1.37

@ronso0 ronso0 force-pushed the waveform-markers-pixmap branch from 7c4855a to 3b5d696 Compare June 3, 2021 18:24
Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Thanks for adding those really helpful and important comments!

What is the difference between or the relation of scaleFactor() and getDevicePixelRatio()? Do we need to take both into account, i.e. multiply them? There is more code that uses WImageStore::getImage() with scaleFactor() and not getDevicePixelRatio().

@ronso0
Copy link
Member Author

ronso0 commented Jun 3, 2021

As far a sI can tell getDevicePixelRatio() is reading the Qt scale factors, which are set either automatically by the OS or manually by the user.

scaleFactor --just guessing here since I'm too lazy to browse the git history-- seems to be a hmm..residue of the Mixxx-internal scaling we had in 2.1(?). See dlgprefinterface.cpp#L184. It's still read from the config but not requested anywhere.
About time to clean that up.
Neither did I find a place where it would be set to anything else than 1.0, nor a place in the GUI that's not scaled correctly (except the cue menu popup https://bugs.launchpad.net/mixxx/+bug/1930751)
getDevicePixelRatioF() seems to be used in the relevant places, for example https://github.com/mixxxdj/mixxx/blob/2.3/src/library/coverartdelegate.cpp#L128-L129

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

I was just curious to see where scaleFactor() is used. It is not documented. Let's ignore this for now and simply only fix what is actually broken.

Thank you for your efforts! LGTM

@uklotzde uklotzde merged commit 6d47e50 into mixxxdj:2.3 Jun 4, 2021
@ronso0 ronso0 deleted the waveform-markers-pixmap branch June 4, 2021 20:01
@ronso0
Copy link
Member Author

ronso0 commented Jun 4, 2021

sure. cleaning up the internal scaling should happen for 2.4, it makes the code more complex than necessary.

@daschuer
Copy link
Member

daschuer commented Jun 5, 2021

Did you try to uncomment the GUI for the old scaleing? By luck it still works.

On the other hand I have the impression we just need a few lines of code to allow to set the QT_SCREEN_SCALE_FACTORS from the GUI and require a restart afterwards.Than we can remove all internal scaling.

We loose the benefit of on the fly scaling and scaling factors lower than 1.

But I think we can effort it.

What do you think?

@ronso0
Copy link
Member Author

ronso0 commented Jun 5, 2021

On the other hand I have the impression we just need a few lines of code to allow to set the QT_SCREEN_SCALE_FACTORS from the GUI and require a restart afterwards.Than we can remove all internal scaling.

Imo that's the way to go now that almost all scalong issues are resolved. internal scaling was an intermediate solution but we must please not reactivate it. just to much trouble composted to Qt scaling.

changing the scaling on the fly is not required IMO, that's mostly a one-time adjustment, isn't it?

@ronso0
Copy link
Member Author

ronso0 commented Jun 5, 2021

that said I have no intention to test the old scaling, and I dont see a use case for it as long as Qt scaling works. do you?

@Holzhaus
Copy link
Member

Holzhaus commented Jun 5, 2021

May this be a regression from this PR: https://bugs.launchpad.net/mixxx/+bug/1930968 ?

@ronso0
Copy link
Member Author

ronso0 commented Jun 5, 2021

maybe. we don't know for sure if the song was broken before on macOS with custom Qt scaling, or if this is a regression on macOS 11.4 only

@ronso0
Copy link
Member Author

ronso0 commented Jun 5, 2021

to revert the fix for macOS I'll set the devicePixelRatio to 1 for macOS only

@ronso0
Copy link
Member Author

ronso0 commented Jun 5, 2021

hmm yeah I think we would have gotten reports about broken markers because some of the folks asking about unscaled Mixxx on Big Sur sure used odd factors with the Qt advice they got

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants