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

Enh sensitivity UI change #278

Merged
merged 31 commits into from
Sep 6, 2024

Conversation

nmarkowitz
Copy link
Contributor

Reference issue

#230 , #212 , #208 , mne-tools/mne-python#10888 , #273

What does this implement/fix?

Extends #273 by enabling sensitivity spinboxes to be updated

@nmarkowitz
Copy link
Contributor Author

@larsoner can you remind me, how can I calculate logical dpi and where should QApplication.primaryScreen().devicePixelRatio() be used for this calculation?

@larsoner
Copy link
Member

larsoner commented Aug 5, 2024

On a HiDPI macOS system for example that has a pixel ratio of 2, making a screen with logical / user size 800x600 you can get the physical pixels by multiplying by the ratio:

>>> from PyQt6 import QtWidgets, QtCore
>>> app = QtWidgets.QApplication([])
>>> window = QtWidgets.QMainWindow(size=QtCore.QSize(800, 600))
>>> window.size().height()
600
>>> window.size().width()
800
>>> window.screen().devicePixelRatio() * window.size().height()
1200.0
>>> window.screen().devicePixelRatio() * window.size().width()
1600.0

@larsoner
Copy link
Member

larsoner commented Aug 7, 2024

Okay some quick feedback:

  • Changing to / inch did not update the Monitor boxes for me
  • Resizing the window did not update the Sensitivity values
  • I think it would be good to insert a third row "Monitor DPI" to the "Monitor Size" section. When any of the three height/width/dpi changes, the other two (and sensitivity values) should update as well.

Let me know when these work for you and I'll take a look! Feel free to add some basic tests as well if you want, we'll want them before merge

@larsoner
Copy link
Member

larsoner commented Aug 7, 2024

... or rather, the wrong stuff updates when I click the / inch in the monitor section. It updates the upper set of boxes, when really it should update the lower set. In other words if I have this to start:

Screenshot 2024-08-07 at 11 32 00 AM

If I click the Monitor Size dropdown / mm and change it to / inch, the sensitivity values shouldn't change, the monitor values should change to 222.52mm/25.4=8.76in, i.e., it changes the units but not the screen size, it doesn't use the old float with the new units to set a new size. So what it does now, which is this, is incorrect:

Screenshot 2024-08-07 at 11 32 28 AM

It should be more like this:

Screenshot 2024-08-07 at 11 33 46 AM

But even when I change it manually to a height of 8.76 (as in the image above) something is wrong, because the grad sensitivity is now 73.6 fT/cm / mm instead of 83.1 fT/cm / mm despite inputting the same physical size for my monitor (granted, in different units, but this should be accounted for properly) and not changing the window size.

And regarding the "no update on resize", I noticed that when I resize I do get this in the terminal:

Traceback (most recent call last):
  File "/Users/larsoner/python/mne-qt-browser/mne_qt_browser/_pg_figure.py", line 5544, in resizeEvent
    self.mne.fig_settings._update_sensitivity_spinbox_values()
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'SettingsDialog' object has no attribute '_update_sensitivity_spinbox_values'

@larsoner larsoner marked this pull request as ready for review August 12, 2024 22:18
@larsoner
Copy link
Member

Just pushed a tiny commit to fix the row number of the Reset button, other than that it worked well!

Still needs tests though

@larsoner larsoner marked this pull request as draft August 12, 2024 22:19
@nmarkowitz
Copy link
Contributor Author

@mscheltienne @larsoner I think this is ready. Can you check out and confirm for yourselves?

@wmvanvliet
Copy link

Functionally, this works very nicely on my Windows machine.

@larsoner
Copy link
Member

larsoner commented Sep 6, 2024

@nmarkowitz I assume you just forgot to take the PR out of draft mode? Works well for me as well so I'll go ahead and merge, thanks!

@larsoner larsoner marked this pull request as ready for review September 6, 2024 14:53
@larsoner larsoner merged commit 1b76f66 into mne-tools:main Sep 6, 2024
19 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.

4 participants