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

Added Settings menu to Main Window with Theme options #2144

Merged
merged 21 commits into from
Apr 12, 2024

Conversation

MikeSullivan7
Copy link
Collaborator

@MikeSullivan7 MikeSullivan7 commented Mar 15, 2024

Issue

closes #2143 and #2099.

Description

A Settings menu has been added via File -> Settings in the Main Window. Clicking on the Settings option opens up a new window where more settings can be added in the future. There is an option to change the current theme of the program to the standard "Fusion" theme or one of the many qt-material themes. By changing the theme, all windows of MI will be updated to the new theme during Runtime so the user can see them in real time. The new settings are saved via QSettings and are checked on startup so once changed to the users liking, it does not need to be changed again!

If "Fusion" is the chosen theme, then a dark mode can be activated by clicking on the "dark mode" checkbox. This checkbox is disabled if any of the qt-material themes are chosen as light/dark is specified in the names of the qt-material themes.

Future work can be done via qt-material to make more custom themes chosen be the user.

Testing

make check

Acceptance Criteria

Theme Testing

  • Open MI and load data in.
  • In the Main Window, go to File -> Settings
  • Change the theme to and check that all open windows update to the new theme during Runtime.
  • With the theme changed, open new windows (recon, spectrum, live viewer, etc) and check that opens in the new theme.
  • Repeat with a few different themes to check that it functions as expected.
  • With the selected theme not being the default "Fusion", exit out of MI and reopen again, the selected theme should remain.
  • All other aspects of MI should function the same as previously.

To simulate a "first time start-up", you need to run QSettings.clear() with the appropriate arguments before starting MI. For example you could add settings.clear() on line 51 of mantidimaging/gui/windows/main/presenter.py, then start MI. This will cause MI to crash on startup but this is good enough to just clear all QSettings for MI. You can then remove the settings.clear() line and start MI as usual.

  • With a first start-up, check that MI starts up with the same theme as you OS.
  • Clear QSettings, change your OS theme and then open a "first startup" MI and check that it now opens with the new theme of your OS.
  • You should be able to now change the theme from within MI, and restart MI where the previously chosen MI theme should remain.

Font Testing

  • The fonts used in MI can now be dynamically changed from the settings menu.
  • Open the settings menu and another window (i.e. operations, SV, etc), change the font size via the settings and check that the fonts are updated across all windows.
  • with the font changed, open a new window and check that the fonts are correct in that window.
  • with the font size changed, close MI and reopen. Check that the font size previously chosen is still active.

OS Theme check

  • on first startup, MI will use OS defaults for font size and dark mode
  • This can be disabled by unchecking the "use OS defaults" checkbox in the Settings Window.
  • After a custom theme or font size has been used, the OS defaults can be toggled on or off via the checkbox

Documentation

Will add release note!

@MikeSullivan7 MikeSullivan7 added Type: Feature Component: GUI UX / GUI EDI Issues which improve equality, diversity or inclusivity to Mantid Imaging labels Mar 15, 2024
@MikeSullivan7 MikeSullivan7 self-assigned this Mar 15, 2024
@coveralls
Copy link

coveralls commented Mar 19, 2024

Coverage Status

coverage: 73.698% (-0.6%) from 74.286%
when pulling 764091e on 2143_Settings_Menu_Themes
into b6087e8 on main.

@MikeSullivan7 MikeSullivan7 added rebuild_docker 🐋 Add if you want to force rebuild docker images (ONLY IF MERGING INTO MAIN) dependencies Pull requests that update a dependency file and removed rebuild_docker 🐋 Add if you want to force rebuild docker images (ONLY IF MERGING INTO MAIN) labels Mar 19, 2024
@samtygier-stfc samtygier-stfc self-requested a review March 20, 2024 13:34
@samtygier-stfc
Copy link
Collaborator

Is it possible to have it pick up the system font on linux? For me I have the system font set to 12 on a first run with this I get the font reduced to 10.
Screenshot at 2024-03-20 13-32-16 main
Screenshot at 2024-03-20 13-42-40 921c790

@samtygier-stfc
Copy link
Collaborator

I am also finding that it gets stuck in dark mode. Not quite sure, but might be to do with the dark mode check box being useOsThemeCheckBox . So maybe if SettingsWindowPresenter.set_theme() sets it true if using fusion.

Also wondering if do_update_UI() is getting qsetting values as strings, so 'if's might not be right.

@samtygier-stfc
Copy link
Collaborator

On IDAaaS you can use xfce4-appearance-settings to change font sizes

@MikeSullivan7
Copy link
Collaborator Author

Printing out the typings used in do_update_UI()

    def do_update_UI(self) -> None:
        theme = settings.value('theme_selection')
        extra_style = settings.value('extra_style')
        os_theme = settings.value('os_theme')
        use_dark_mode = settings.value('use_dark_mode')
        override_os_theme = settings.value('override_os_theme')
        print(f"theme type: {type(theme)}")
        print(f"extra_style: {type(extra_style)}")
        print(f"os_theme: {type(os_theme)}")
        print(f"use_dark_mode: {type(use_dark_mode)}")
        print(f"override_os_theme: {type(override_os_theme)}")

gives:

theme type: <class 'str'>
extra_style: <class 'dict'>
os_theme: <class 'str'>
use_dark_mode: <class 'int'>
override_os_theme: <class 'str'>

so using if use_dark_mode: is fine but would be better to set as a bool or specify ==0 or ==1 etc

Do you know the steps to reproduce getting stuck in dark mode on Linux as I cant seem to reproduce it in Windows.
I'll try using IDaaS to reproduce it

@samtygier-stfc
Copy link
Collaborator

Steps to reproduce on linux.
Delete Mantid\ Imaging.conf Mantid\ Imaging.conf
Open MI, open settings.
Tick and then untick Dark mode.
Quit MI.
Reopen MI, it will be back in dark mode.

This might be because the qsettings in linux are in ini/toml format, so don't have a type and are treated as strings by default.

@MikeSullivan7
Copy link
Collaborator Author

Checking the settings type in linux I get:

theme type: <class 'str'>
extra_style: <class 'dict'>
os_theme: <class 'str'>
use_dark_mode: <class 'str'>
override_os_theme: <class 'str'>

so looks like it would be a better idea to store the settings as strictly strings to be consistent in Linux and Windows, good catch!

@MikeSullivan7 MikeSullivan7 added rebuild_docker 🐋 Add if you want to force rebuild docker images (ONLY IF MERGING INTO MAIN) and removed rebuild_docker 🐋 Add if you want to force rebuild docker images (ONLY IF MERGING INTO MAIN) labels Apr 3, 2024
@MikeSullivan7
Copy link
Collaborator Author

The default font family and size are found via QFont and then stored in QSettings. For Windows, this is "MS Shell Dlg 2" and font size 12. Once the font size is changed in the settings, the default size is overridden. May be an idea to employ a "Return to defaults" button in the future.

Current QSettings are:

image

They are stored in Computer\HKEY_CURRENT_USER\SOFTWARE\mantidproject\Mantid Imaging in Windows and in a .conf file on Linux.

if theme_selection:
q_application.setStyle(settings.value('theme_selection'))

default_font = QFont('-1')
Copy link
Collaborator

Choose a reason for hiding this comment

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

For me on linux I needed to change this to just default_font = QFont() to get the OS value. With the -1 it will always give 12, Noto Sans.

QFontInfo(QFont('-1')).pointSize()=12
QFontInfo(QFont('-1')).family()='Noto Sans'
QFontInfo(QFont()).pointSize()=14
QFontInfo(QFont()).family()='Times New Roman'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using QFont() on windows gives the default pointsize as 8, even when the scalling is changed and the Main Window looks like this:

image

(mantidimaging-dev) C:\Users\ddb29996\mantidimaging>python -m mantidimaging
default_font_info.pointSize: 8
default_font_info.family: MS Shell Dlg 2

It seems like grabbing the default font is something that is only useful for Linux as the scaling options in windows seem to take care of it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm happy for the check to only be on linux. One last thing to try is getting it from a widget.

QWidget().font().pointSize()
QWidget().font().family()

Copy link
Collaborator

@samtygier-stfc samtygier-stfc left a comment

Choose a reason for hiding this comment

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

Could some of the logic be made simpler if there was a "Use system theme" check box.

@MikeSullivan7
Copy link
Collaborator Author

Could some of the logic be made simpler if there was a "Use system theme" check box.

This could be a solution because the extra_style stylesheet is getting written before the Qfont in main.py is constructed, so then the font sizes are fighting against each other.

Using a "Use system theme" check box which is checked on first startup and then forces the defaults to be used would simplify things

@MikeSullivan7
Copy link
Collaborator Author

In Windows, setting the monitor to a scaling of 100% (with 250% recommended) leads to

default_font_info.pointSize: 16
default_font_info.family: MS Shell Dlg 2
default_Qwidget.fontInfo().pointSize: 16

Setting the same monitor to 400% leads to:

default_font_info.pointSize: 4
default_font_info.family: MS Shell Dlg 2
default_Qwidget.fontInfo().pointSize: 4

where the font size is the same relative to the text in other programs on that monitor.

@samtygier-stfc
Copy link
Collaborator

So I guess use default_font_info to get the system font on linux only

@MikeSullivan7
Copy link
Collaborator Author

I think its fine to use on both Windows and Linux, Windows seems to handle the fonts well as it interacts with the scaling well. It looks like it just uses the font size as one relative to the scaling rather than an "absolute" font size

@MikeSullivan7
Copy link
Collaborator Author

Testing on Linux

image

image

Mantid Imaging.conf:

[General]
default_font_family=DejaVu Sans
default_font_size=11
extra_style=@Variant(\0\0\0\b\0\0\0\x2\0\0\0\x12\0\x66\0o\0n\0t\0_\0s\0i\0z\0\x65\0\0\0\n\0\0\0\b\0\x31\0\x31\0p\0x\0\0\0\x1a\0\x64\0\x65\0n\0s\0i\0t\0y\0_\0s\0\x63\0\x61\0l\0\x65\0\0\0\n\0\0\0\x4\0-\0\x35)
os_theme=Light
theme_selection=Fusion

[welcome_screen]
last_run_version=0.0.0.dev1
show_at_start=true

The OS font is now being displayed correctly on Linux

@MikeSullivan7 MikeSullivan7 added rebuild_docker 🐋 Add if you want to force rebuild docker images (ONLY IF MERGING INTO MAIN) and removed rebuild_docker 🐋 Add if you want to force rebuild docker images (ONLY IF MERGING INTO MAIN) labels Apr 9, 2024
Copy link
Collaborator

@samtygier-stfc samtygier-stfc left a comment

Choose a reason for hiding this comment

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

This is working much better now. Just a couple of small code issues, and the performance tab.

I see some sizing issues if I use the material theme. And there seems to be case where some text in the image views does not adjust. But happy to merge once commented issues are done, and deal with these later.

Comment on lines +16 to +19
settings = QtCore.QSettings('mantidproject', 'Mantid Imaging')

if not settings.contains("theme_selection") or settings.value("theme_selection") is None:
settings.setValue('theme_selection', 'Fusion')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be here, there is similar code in mantidimaging/main.py.

</layout>
</widget>
</widget>
<widget class="QWidget" name="performanceTab">
Copy link
Collaborator

Choose a reason for hiding this comment

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

The performance tabs seems to have snuck into this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this happened because I had the .ui file open in QT Designer and it didnt switch properly when changing between the two branches, i'll fix it now

Comment on lines +35 to +36
settings = QSettings('mantidproject', 'Mantid Imaging')

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is used

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

settings is used in do_update_UI()

    def do_update_UI(self) -> None:
        if settings.value('use_os_defaults', defaultValue='True') == 'True':
            extra_style = settings.value('extra_style_default')
            theme = 'Fusion'
            override_os_theme = 'False'
        else:
            extra_style = settings.value('extra_style')
            use_dark_mode = settings.value('use_dark_mode')
            theme = settings.value('theme_selection')
            override_os_theme = settings.value('override_os_theme')

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I put the comment in the wrong place, should have been in view.py line 58. I think that one is not used

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes you're right, thats a remnant of me refactoring some things from the view to the presenter

@MikeSullivan7
Copy link
Collaborator Author

Yes there are some positioning and sizing issues when using the qt_material themes but to fix this would take some modification of the themes style sheets which may take some time. I think it would be best to note that these themes are "experimental" and come back to it at a later PR

Copy link
Collaborator

@samtygier-stfc samtygier-stfc left a comment

Choose a reason for hiding this comment

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

Thanks

@samtygier-stfc samtygier-stfc added this pull request to the merge queue Apr 12, 2024
Merged via the queue into main with commit ea787c4 Apr 12, 2024
8 checks passed
@samtygier-stfc samtygier-stfc deleted the 2143_Settings_Menu_Themes branch April 12, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: GUI dependencies Pull requests that update a dependency file EDI Issues which improve equality, diversity or inclusivity to Mantid Imaging rebuild_docker 🐋 Add if you want to force rebuild docker images (ONLY IF MERGING INTO MAIN) Type: Feature UX / GUI
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Theme Selection through a Settings Menu
3 participants