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

citra-qt : Adding fullscreen mode #3001

Merged
merged 1 commit into from Oct 19, 2017

Conversation

Projects
None yet
9 participants
@Styleoshin
Copy link
Contributor

Styleoshin commented Oct 8, 2017

I added mode fullscreen for play with more comfort.
For display fullscreen, just click on action bar or the shortcut : alt+return

1

6

2

For exit fullscreen just click in alt+return or escape

3


This change is Reviewable

@valentinvanelslande

This comment has been minimized.

Copy link
Contributor

valentinvanelslande commented Oct 8, 2017

Submodule citra?

@j-selby
Copy link
Contributor

j-selby left a comment

Looking pretty good.

@@ -45,7 +45,7 @@
<x>0</x>
<y>0</y>
<width>1081</width>
<height>19</height>
<height>21</height>

This comment has been minimized.

@j-selby

j-selby Oct 8, 2017

Contributor

Not sure why Qt Designer keeps adding this. Unintentional change?

This comment has been minimized.

@Styleoshin

Styleoshin Oct 9, 2017

Contributor

Yes, Qt Designer added it

void GMainWindow::keyPressEvent(QKeyEvent* event) {

if (event->key() == Qt::Key_Return) {
if (event->modifiers() && Qt::AltModifier) {

This comment has been minimized.

@j-selby

j-selby Oct 8, 2017

Contributor

This isn't doing what you think it is. & is the bitwise operator (which is likely what you are looking for).

Example fix:
(event->modifiers() & Qt::AltModifier) != 0

</widget>
<resources/>
<connections/>

This comment has been minimized.

@j-selby

j-selby Oct 8, 2017

Contributor

Is this needed?


// Exit fullscreen in GMainWindow
if (event->key() == Qt::Key_Return) {
if (event->modifiers() && Qt::AltModifier) {

This comment has been minimized.

@j-selby

j-selby Oct 8, 2017

Contributor

See below comment on this.

@j-selby

This comment has been minimized.

Copy link
Contributor

j-selby commented Oct 8, 2017

"canno't enter in fullscreen if single window mode is not checked"

This shouldn't be a hidden note in this PR. Is there any reason why this is not the case?

@@ -624,6 +626,20 @@ void GMainWindow::OnStopGame() {
ShutdownGame();
}

void GMainWindow::OnDisplayFullscreen() {
if (ui.action_Single_Window_Mode->isChecked()) {

This comment has been minimized.

@lioncash

lioncash Oct 9, 2017

Member

It's possible to unindent this by comparing in terms of the early-exit case first.

void GMainWindow::OnDisplayFullscreen() {
    if (!ui.action_Single_Window_Mode->isChecked()) {
        return;
    }

    ui.menubar->hide();
    statusBar()->hide();
    showFullScreen();
}

@Styleoshin Styleoshin force-pushed the Styleoshin:qt_fullscreen branch from d85d663 to 108f34c Oct 9, 2017

@Styleoshin

This comment has been minimized.

Copy link
Contributor

Styleoshin commented Oct 10, 2017

Now it's possible to play fullscreen on single or separate window

@jroweboy
Copy link
Member

jroweboy left a comment

Okay. First off, thanks again for the contribution <3 I really appreciate it.

One question I have, is does it go into fullscreen only when a game is running? Or will it try to fullscreen the game list? From the looks of it, it looks like it'll go fullscreen on the game list if you click the menu action? I think the expected behavior is

The rest of the code is going to need some work. <3 GRenderWindow should not be used for generic frontend things like "fullscreen" or whatever. All of that should be put on the GMainWindow. To help you get started, heres some sample code you can use: (it might not compile but it should be good enough to get started)

// setup Esc to exit fullscreen mode
shortcut = new QShortcut(QKeySequence(tr("Esc", "Exit Fullscreen")), parent);
connect(shortcut, &QShortcut::activated(), ui.actionFullscreen, QAction::setChecked(false));

// or you can use a lambda and check if the game is running or something
connect(shortcut, &QShortcut::activated(), ui.actionFullscreen, [&] { if (game is running) ui.actionFullscreen->setChecked(false); });

...

void GMainWindow::ToggleFullscreen() {
    if (game isn't running)
        return;
    if (ui.action_Fullscreen->isChecked()) {
        if (single window mode) {
		    grenderwindow->showFullScreen()
		} else {
		    this->fullscreen
		}
    } else {
        //do the opposite
    }
}

// and then on startup after the game starts running
ToggleFullscreen(); // to open the game as full screen if its selected

// the ToggleFullscreen should still be connected to the QAction triggered signal like you already did.

Hope this helps. As always, I'm available for questions on IRC and discord.

void GRenderWindow::closeEvent(QCloseEvent* event) {
emit Closed();
QWidget::closeEvent(event);
}

void GRenderWindow::keyPressEvent(QKeyEvent* event) {
InputCommon::GetKeyboard()->PressKey(event->key());

This comment has been minimized.

@jroweboy

jroweboy Oct 10, 2017

Member

This is the wrong place to put this check. See the final review comment for the full details.

@@ -201,13 +201,37 @@ qreal GRenderWindow::windowPixelRatio() {
#endif
}

void GRenderWindow::SeparateFromMainWindow(bool separate) {

This comment has been minimized.

@jroweboy

jroweboy Oct 10, 2017

Member

After moving the code back to GMainWindow, we won't need any of this variable anymore

@Styleoshin Styleoshin force-pushed the Styleoshin:qt_fullscreen branch from 108f34c to 8cc0251 Oct 10, 2017

@jroweboy

This comment has been minimized.

Copy link
Member

jroweboy commented Oct 11, 2017

@Styleoshin i went ahead and added some more fixes to my branch. This will register the Fullscreen hotkey in the menu as well, so that way people will know what the shortcut is. Its also registered with the hotkey system in citra, so people can change the hotkey through the settings if they want. https://github.com/jroweboy/citra/tree/fullscreen

@Styleoshin

This comment has been minimized.

Copy link
Contributor

Styleoshin commented Oct 11, 2017

ok, i see, that's a good idea :)

@jroweboy

This comment has been minimized.

Copy link
Member

jroweboy commented Oct 11, 2017

@Styleoshin go ahead and add what I wrote to your PR however you want. I just didn't want to have a back and forth about "Thats not quite what I meant" and just wrote out what I was expecting. Also when I sat down to work on this, I didn't realize how tricky having fullscreen as a menu action made it :P

@Styleoshin Styleoshin force-pushed the Styleoshin:qt_fullscreen branch from 8cc0251 to e3d9e6c Oct 11, 2017

@Styleoshin

This comment has been minimized.

Copy link
Contributor

Styleoshin commented Oct 11, 2017

You're right, it's better

@jroweboy

This comment has been minimized.

Copy link
Member

jroweboy commented Oct 18, 2017

Last call for reviews before I merge it in 24 hours

@jroweboy jroweboy merged commit f47bf6c into citra-emu:master Oct 19, 2017

2 of 3 checks passed

code-review/reviewable 5 files, 7 discussions left (j-selby, jroweboy, lioncash)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@FearlessTobi

This comment has been minimized.

Copy link
Contributor

FearlessTobi commented Oct 21, 2017

In builds with this PR merged, I get an annoying bug. When I am on the Game List and my Citra window is maximized and I now start a game, my Citra window will minimize. Here´s a GIF for better explanation:

ezgif com-video-to-gif

@jroweboy

This comment has been minimized.

Copy link
Member

jroweboy commented Oct 21, 2017

Ah this is a regression because GMainWindow::ToggleFullscreen doesn't track what state it was in (Maximized, Minimized, etc) and just returns the window to "Normal" if the window isn't fullscreened. The fix shouldn't be that hard to do if anyone has some free time :)

@RavenHome1

This comment has been minimized.

Copy link

RavenHome1 commented Oct 21, 2017

actually while we are on the topic of full screen mode

  • the status bar (the thing that shows speed % and frame rate) completely vanishes and it would be nice if you can maybe move it to the corner so it can keep displaying these value.

  • using full screen mode causes a lot of tearing to happen, not sure if something that can't be helped without V-sync since i have it off but maybe look into it

@Yogarpg

This comment has been minimized.

Copy link

Yogarpg commented Feb 6, 2018

Why my emulation background is white, even in fullscreen?
Black is so much better
captura de tela 267

@jroweboy

This comment has been minimized.

Copy link
Member

jroweboy commented Feb 6, 2018

@Yogarpg this was already change from white to black back in june #2795 the background color is a customizable setting in the config ini. Please ask on the forums for more help if you need it, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment