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 set_fullscreen API to WindowAdapter. #4286

Merged
merged 3 commits into from Jan 9, 2024

Conversation

qhua948
Copy link
Contributor

@qhua948 qhua948 commented Jan 8, 2024

This PR exposes the fullscreen API for both winit and qt to WindowAdapter which can be called from client code to set or unset fullscreen status.

Removed inflexible env variable SLINT_FULLSCREEN and instead use the new API to set fullscreen status.

Added an example application examples/fullscreen_toggle to demo the toggle.

Note that this is a breaking change, if the maintainer believes that the environment variable should be kept, then I am happy to amend this as well.

See #3283

@CLAassistant
Copy link

CLAassistant commented Jan 8, 2024

CLA assistant check
All committers have signed the CLA.

@qhua948 qhua948 force-pushed the fullscreen-toggle branch 2 times, most recently from 41a3195 to 991b11a Compare January 8, 2024 02:16
Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR! It is well done.

I added a few comments inline.
Also I don't think it is necessary to have a full fledged example just for that feature.

@@ -203,11 +203,6 @@ impl WinitWindowAdapter {
let mut window_builder =
winit::window::WindowBuilder::new().with_transparent(true).with_visible(false);

if std::env::var("SLINT_FULLSCREEN").is_ok() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking we should keep the env variable for now in addition, or that'd be a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 1704 to 1716
if (fullscreen) {
widget_ptr->setWindowState(Qt::WindowFullScreen);
} else {
widget_ptr->setWindowState(Qt::WindowNoState);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This would also clear other states like minimized and maximized.
In fact, the best would be to add or remove the Qt::WindowFullScreen flag from the set of state using windowStates and setWindowStates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes a lot of sense, done.

@hunger
Copy link
Member

hunger commented Jan 8, 2024

The C++ formatting is broken:-/ Could you please consider to fix that?

Unfortunately github will not push fix suggestions into other people's branches, so unfortunately you can not just accept that :-) And neither can I do it for you as I can not push an update into your branch either.

@qhua948 qhua948 force-pushed the fullscreen-toggle branch 2 times, most recently from 690b6c3 to 84acb51 Compare January 8, 2024 19:29
@qhua948
Copy link
Contributor Author

qhua948 commented Jan 8, 2024

The C++ formatting is broken:-/ Could you please consider to fix that?

Unfortunately github will not push fix suggestions into other people's branches, so unfortunately you can not just accept that :-) And neither can I do it for you as I can not push an update into your branch either.

Just ran clang-format :3

@qhua948 qhua948 force-pushed the fullscreen-toggle branch 5 times, most recently from 79d9b59 to 7ab4869 Compare January 8, 2024 20:32
env var and related docs.

Use `QWindow::setWindowStates()` to set the fullscreen state for Qt
backend.
Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the patch!

@tronical tronical merged commit 779aff0 into slint-ui:master Jan 9, 2024
35 checks passed
@omac777
Copy link

omac777 commented Mar 6, 2024

I was looking for examples/fullscreen_toggle/
I didn't find it after I did a git pull.

git describe --tags
v1.3.0-65-ga3e48c495

None of the above is visible to me.
41a3195

git clone https://github.com/slint-ui/slint
Cloning into 'slint'...
remote: Enumerating objects: 115264, done.
remote: Counting objects: 100% (2917/2917), done.
remote: Compressing objects: 100% (1332/1332), done.
remote: Total 115264 (delta 1790), reused 2373 (delta 1529), pack-reused 112347
Receiving objects: 100% (115264/115264), 30.64 MiB | 26.29 MiB/s, done.
Resolving deltas: 100% (81610/81610), done.
cissy@cissy-flex5-fc38sb:$ cd slint/examples/
cissy@cissy-flex5-fc38sb:
/slint/examples$ ls
7guis ffmpeg opengl_texture slide_puzzle
bash gallery opengl_underlay todo
carousel gstreamer-player plotter uefi-demo
CMakeLists.txt imagefilter printerdemo virtual_keyboard
cpp iot-dashboard printerdemo_mcu
energy-monitor mcu-board-support printerdemo_old
fancy_demo memory README.md
cissy@cissy-flex5-fc38sb:~/slint/examples$ git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean

fullscreen_toggle directory in examples is still not there. I don't understand. I thought this was committed and part of master.

@omac777
Copy link

omac777 commented Mar 6, 2024

I just went to the details for this merge/commit.
https://github.com/slint-ui/slint/actions/runs/7456944726/job/20288395478
I don't see anywhere in those details mentioning it building fullscreen_toggle.

Am I blind? What were those builds and tests using to evaluate that it passed?

@qhua948
Copy link
Contributor Author

qhua948 commented Mar 6, 2024 via email

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.

None yet

6 participants