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

go fullscreen early #3844

Merged
merged 1 commit into from May 11, 2021
Merged

go fullscreen early #3844

merged 1 commit into from May 11, 2021

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented May 8, 2021

window decoration became obvious with hiding the menubar during startup #3839
Let's wrap up the startup polishing and go fullscreen early.
Now we have a clean launch screen, then the skin shows up with the styled menubar.

This also fixes the bug that [ ] Fullscreen is unchecked after startup even when we are fullscreen.
https://bugs.launchpad.net/mixxx/+bug/1579493

For full enjoyment merge #3839 on top : )

@ronso0 ronso0 added this to In progress in 2.3 release via automation May 8, 2021
@ronso0 ronso0 added this to the 2.3.0 milestone May 8, 2021
@ronso0 ronso0 moved this from In progress to Needs Review in 2.3 release May 8, 2021
@JoergAtGithub
Copy link
Member

I tested this on Windows7 and it works seamless without any glitch!

@ronso0
Copy link
Member Author

ronso0 commented May 8, 2021

@ronso0
Copy link
Member Author

ronso0 commented May 8, 2021

whoops, pushed with the necessary Fullscreen checkbox update just now.

Ready for review.
@daschuer @Holzhaus

@Holzhaus
Copy link
Member

Holzhaus commented May 8, 2021

If I tick "Start in Fullscreen", i'm getting SIGSEGV:

─── Output/messages ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────

Thread 1 "mixxx" received signal SIGSEGV, Segmentation fault.
0x00007ffff3db14b3 in QObjectPrivate::connectImpl(QObject const*, int, QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, QMetaObject const*) () from /usr/lib/libQt5Core.so.5
─── Assembly ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
0x00007ffff3db14a7 ? mov    %edx,0x54(%rax)
0x00007ffff3db14aa ? movups %xmm0,0x28(%rax)
0x00007ffff3db14ae ? mov    0x8(%r12),%rax
0x00007ffff3db14b3 ? mov    0x38(%rax),%rbx
0x00007ffff3db14b7 ? mov    %rbx,%rdi
0x00007ffff3db14ba ? addr32 call 0x7ffff3b993c0
0x00007ffff3db14c0 ? mov    0x30(%rsp),%rax
─── Expressions ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
─── History ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
─── Memory ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
─── Registers ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   rax 0x840f02a8f8408b48     rbx 0x00007fffffffd2f0     rcx 0x0000000000000000     rdx 0x000000008000000d     rsi 0x0000000000000058
   rdi 0x00005555568de700     rbp 0x00007fffdc006ac0     rsp 0x00007fffffffd1d0      r8 0x00005555568de700      r9 0x00007ffff36eba60
   r10 0x00005555568dec60     r11 0x0000000000000000     r12 0x00007ffff35b6435     r13 0x000000000000000d     r14 0x00007fffffffd3f8
   r15 0x0000555556254dc0     rip 0x00007ffff3db14b3  eflags [ SF IF RF ]            cs 0x00000033              ss 0x0000002b
    ds 0x00000000              es 0x00000000              fs 0x00000000              gs 0x00000000
─── Source ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
─── Stack ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
[0] from 0x00007ffff3db14b3 in QObjectPrivate::connectImpl(QObject const*, int, QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, QMetaObject const*)
(no arguments)
[1] from 0x00007ffff3db19f6 in QObject::connectImpl(QObject const*, void**, QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, QMetaObject const*)
(no arguments)
[+]
─── Threads ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
[29] id 277706 name mixxx:shlo4 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[28] id 277705 name mixxx:shlo3 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[27] id 277704 name mixxx:shlo2 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[26] id 277703 name mixxx:shlo1 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[25] id 277702 name mixxx:shlo0 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[24] id 277701 name mixxx:sh11 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[23] id 277700 name mixxx:sh10 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[22] id 277699 name mixxx:sh9 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[21] id 277698 name mixxx:sh8 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[20] id 277697 name mixxx:sh7 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[19] id 277696 name mixxx:sh6 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[18] id 277695 name mixxx:sh5 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[17] id 277694 name mixxx:sh4 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[16] id 277693 name mixxx:sh3 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[15] id 277692 name mixxx:sh2 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[14] id 277691 name mixxx:sh1 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[13] id 277690 name mixxx:sh0 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[12] id 277689 name mixxx:disk$3 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[11] id 277688 name mixxx:disk$2 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[10] id 277687 name mixxx:disk$1 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[9] id 277686 name mixxx:disk$0 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[8] id 277685 name mixxx:cs0 from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[7] id 277684 name mixxx from 0x00007ffff370e9ba in __futex_abstimed_wait_common64+202
[6] id 277683 name QDBusConnection from 0x00007ffff361e37f in poll+79
[5] id 277682 name gdbus from 0x00007ffff361e37f in poll+79
[4] id 277681 name dconf worker from 0x00007ffff361e37f in poll+79
[3] id 277680 name gmain from 0x00007ffff361e37f in poll+79
[2] id 277679 name QXcbEventQueue from 0x00007ffff361e37f in poll+79
[1] id 277674 name mixxx from 0x00007ffff3db14b3 in QObjectPrivate::connectImpl(QObject const*, int, QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, QMetaObject const*)
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
>>> bt
#0  0x00007ffff3db14b3 in QObjectPrivate::connectImpl(QObject const*, int, QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, QMetaObject const*) () at /usr/lib/libQt5Core.so.5
#1  0x00007ffff3db19f6 in QObject::connectImpl(QObject const*, void**, QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, QMetaObject const*) () at /usr/lib/libQt5Core.so.5
#2  0x0000555555723d77 in QObject::connect<void (WMainMenuBar::*)(), void (TrackCollectionManager::*)()>(QtPrivate::FunctionPointer<void (WMainMenuBar::*)()>::Object const*, void (WMainMenuBar::*)(), QtPrivate::FunctionPointer<void (TrackCollectionManager::*)()>::Object const*, void (TrackCollectionManager::*)(), Qt::ConnectionType) (type=Qt::AutoConnection, slot=(void (TrackCollectionManager::*)(TrackCollectionManager * const)) 0x555555b27260 <TrackCollectionManager::startLibraryScan()>, receiver=0x7ffff35b6435 <calloc+133>, signal=(void (WMainMenuBar::*)(WMainMenuBar * const)) 0x55555589f690 <WMainMenuBar::rescanLibrary()>, sender=0x7fffdc006ac0) at /usr/include/qt/QtCore/qobject.h:268
#3  MixxxMainWindow::connectMenuBar() (this=0x7fffffffdb90) at /home/jan/Projects/mixxx/src/mixxx.cpp:1273
#4  0x00005555557241ad in MixxxMainWindow::slotViewFullScreen(bool) (this=this@entry=0x7fffffffdb90, toggle=toggle@entry=true) at /home/jan/Projects/mixxx/src/mixxx.cpp:1395
#5  0x000055555572ba55 in MixxxMainWindow::initialize(QApplication*, CmdlineArgs const&) (this=0x7fffffffdb90, pApp=0x7fffffffdb50, args=...) at /home/jan/Projects/mixxx/src/mixxx.cpp:261
#6  0x000055555572d1f3 in MixxxMainWindow::MixxxMainWindow(QApplication*, CmdlineArgs const&) (this=0x7fffffffdb90, pApp=0x7fffffffdb50, args=...) at /home/jan/Projects/mixxx/src/mixxx.cpp:226
#7  0x00005555556fbaaa in (anonymous namespace)::runMixxx (args=..., app=0x7fffffffdb50) at /home/jan/Projects/mixxx/src/main.cpp:29
#8  main(int, char**) (argc=<optimized out>, argv=<optimized out>) at /home/jan/Projects/mixxx/src/main.cpp:110
>>>

@ronso0
Copy link
Member Author

ronso0 commented May 8, 2021

uff, no idea where this is coming from. I'll investigate

@ronso0
Copy link
Member Author

ronso0 commented May 9, 2021

@Holzhaus
rebased, and the menubar slot is now called directly.
Please try again.
Though I don't understand why it should fail with the previous solution since.

@Holzhaus
Copy link
Member

Holzhaus commented May 9, 2021

Still crashes for me:

>>> bt
#0  0x00007ffff3db14b3 in QObjectPrivate::connectImpl(QObject const*, int, QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, QMetaObject const*) () at /usr/lib/libQt5Core.so.5
#1  0x00007ffff3db19f6 in QObject::connectImpl(QObject const*, void**, QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, QMetaObject const*) () at /usr/lib/libQt5Core.so.5
#2  0x0000555555723de7 in QObject::connect<void (WMainMenuBar::*)(), void (TrackCollectionManager::*)()>(QtPrivate::FunctionPointer<void (WMainMenuBar::*)()>::Object const*, void (WMainMenuBar::*)(), QtPrivate::FunctionPointer<void (TrackCollectionManager::*)()>::Object const*, void (TrackCollectionManager::*)(), Qt::ConnectionType) (type=Qt::AutoConnection, slot=(void (TrackCollectionManager::*)(TrackCollectionManager * const)) 0x555555b273c0 <TrackCollectionManager::startLibraryScan()>, receiver=0x7ffff35b6435 <calloc+133>, signal=(void (WMainMenuBar::*)(WMainMenuBar * const)) 0x55555589f730 <WMainMenuBar::rescanLibrary()>, sender=0x7fffdc006bc0) at /usr/include/qt/QtCore/qobject.h:268
#3  MixxxMainWindow::connectMenuBar() (this=0x7fffffffdc10) at /home/jan/Projects/mixxx/src/mixxx.cpp:1275
#4  0x000055555572421d in MixxxMainWindow::slotViewFullScreen(bool) (this=this@entry=0x7fffffffdc10, toggle=toggle@entry=true) at /home/jan/Projects/mixxx/src/mixxx.cpp:1397
#5  0x000055555572badd in MixxxMainWindow::initialize(QApplication*, CmdlineArgs const&) (this=0x7fffffffdc10, pApp=0x7fffffffdbd0, args=...) at /home/jan/Projects/mixxx/src/mixxx.cpp:262
#6  0x000055555572d28f in MixxxMainWindow::MixxxMainWindow(QApplication*, CmdlineArgs const&) (this=0x7fffffffdc10, pApp=0x7fffffffdbd0, args=...) at /home/jan/Projects/mixxx/src/mixxx.cpp:227
#7  0x00005555556fbafa in (anonymous namespace)::runMixxx (args=..., app=0x7fffffffdbd0) at /home/jan/Projects/mixxx/src/main.cpp:29
#8  main(int, char**) (argc=<optimized out>, argv=<optimized out>) at /home/jan/Projects/mixxx/src/main.cpp:110
>>>

@Holzhaus
Copy link
Member

Holzhaus commented May 9, 2021

Maybe because we recreate the menu bar every time we open fullscreen and overwrite the pointer?

@ronso0
Copy link
Member Author

ronso0 commented May 9, 2021

that doesn't happen during startup, only when toggling fullscreen later on, doesn't it?

@Holzhaus
Copy link
Member

Holzhaus commented May 9, 2021

No, you call slotViewFullscreen(true) during startup and thr menu bar is recreated: https://github.com/ronso0/mixxx/blob/early-fullscreen/src/mixxx.cpp#L1396

@Holzhaus
Copy link
Member

Holzhaus commented May 9, 2021

commenting out createMenuBar doesn't solve issue. Commenting out connectMenuBar too works fine though:

$ git diff
diff --git a/src/mixxx.cpp b/src/mixxx.cpp
index 7c127aea5d..35e3b25d10 100644
--- a/src/mixxx.cpp
+++ b/src/mixxx.cpp
@@ -1393,16 +1393,16 @@ void MixxxMainWindow::slotViewFullScreen(bool toggle) {
         // Fix for "No menu bar with ubuntu unity in full screen mode" Bug
         // #885890 and Bug #1076789. Before touching anything here, please read
         // those bugs.
-        createMenuBar();
-        connectMenuBar();
+        //createMenuBar();
+        //connectMenuBar();
         if (m_pMenuBar->isNativeMenuBar()) {
             m_pMenuBar->setNativeMenuBar(false);
         }
 #endif
     } else {
 #ifdef __LINUX__
-        createMenuBar();
-        connectMenuBar();
+        //createMenuBar();
+        //connectMenuBar();
 #endif
         showNormal();
     }

However, I have no idea about the referenced bug report.

@ronso0
Copy link
Member Author

ronso0 commented May 9, 2021

No, you call slotViewFullscreen(true) during startup and thr menu bar is recreated: https://github.com/ronso0/mixxx/blob/early-fullscreen/src/mixxx.cpp#L1396

right, my bad. This is unnecessary in the first place because we don't need to toggle, just go fullscreen directly with showFullScreen() and emit fullScreenChanged(isFullScreen()); lke in the first commit.

Though I wonder why it works for me with Qt 5.12.8 and on Win7...
Please test again.

@JoergAtGithub
Copy link
Member

Though I wonder why it works for me with Qt 5.12.8 and on Win7...

The later because these statements are encapsulated in #ifdef __LINUX__

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Works fine, thank you. Should I merge or do you want more testing first?

In the long term, I think we should remove the "Start in Fullscreen" option and just remember if fullscreen was enabled when mixxx is closed. Not for 2.3 though.

@ronso0
Copy link
Member Author

ronso0 commented May 9, 2021

I want to squash and then testing in MATE 20 would be good which is currently the only contemporary distro that has a global menu out of the box. Apart from that it seems 'global menu' is supplied only 3rd-party extensions.

@ronso0 ronso0 marked this pull request as draft May 9, 2021 21:32
@ronso0
Copy link
Member Author

ronso0 commented May 9, 2021

In the long term, I think we should remove the "Start in Fullscreen" option and just remember if fullscreen was enabled when mixxx is closed. Not for 2.3 though.

hehee, sure. so effectively revert #3493, just that it works for everyone : )

@ronso0
Copy link
Member Author

ronso0 commented May 9, 2021

I want to squash and then testing in MATE 20 would be good which is currently the only contemporary distro that has a global menu out of the box. Apart from that it seems 'global menu' is supplied only 3rd-party extensions.

didn't try MATE but even there it seems to be an optional extension. latest findings (yet not tested by me): only KDE plasma has native global menu capabilities, though it's not enabled.

Anyhow, long story short: I re-enabled my Xfce-appmenu plugin (disabled for styling the Mixxx menubar) and there are no issues. menu is populated all the time, all shortcuts work.

Sqashed, ready for merge. Thanks for testing on short notice @Holzhaus @JoergAtGithub !

@ronso0 ronso0 marked this pull request as ready for review May 9, 2021 22:49
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@Holzhaus Holzhaus merged commit 6bdfb6b into mixxxdj:2.3 May 11, 2021
2.3 release automation moved this from Needs Review to Done May 11, 2021
@Holzhaus
Copy link
Member

@ronso0 can you resolve the merge conflicts with main?

@uklotzde
Copy link
Contributor

I've just resolved the conflicts for testing this PR with main. Will do the merge if there are no other conflicts occur.

@ronso0
Copy link
Member Author

ronso0 commented May 11, 2021

Great, thanks!

@ronso0 ronso0 deleted the early-fullscreen branch May 11, 2021 17:23
@uklotzde
Copy link
Contributor

Unrelated: Switching from fullscreen into windowing mode on Fedora 34//GNOME 40/Wayland sometimes (not always) messes up the client-side decorations by misplacing the window contents.

Screenshot from 2021-05-11 19-13-11

Unfortunately (or luckily), I am not able to reproduce it.

@uklotzde
Copy link
Contributor

I had the move the initialization of m_pLibraryExporter before switching to fullscreen.

@uklotzde
Copy link
Contributor

Done, please retest.

@uklotzde
Copy link
Contributor

uklotzde commented May 11, 2021

Also (maybe?) unrelated but a serious bug: connectMenuBar() is invoked multiple times now and establishes duplicate signal/slot connections!! Could easily be verified by opening the About dialogue.

This has probably been messed up by a previous PR that tweaked the initialization order. We need to use unique connections if we cannot prevent repeated invocations.

@uklotzde
Copy link
Contributor

And clazy is complaining, I didn't notice that:

https://github.com/mixxxdj/mixxx/runs/2558341753?check_suite_focus=true

Error: /home/runner/work/mixxx/mixxx/src/mixxx.cpp:232:10: error: Emitting inside constructor probably has no effect [-Wclazy-incorrect-emit]
    emit fullScreenChanged(isFullScreen());
         ^
1 error generated.
make[2]: *** [CMakeFiles/mixxx-lib.dir/build.make:4122: CMakeFiles/mixxx-lib.dir/src/mixxx.cpp.o] Error 1

@ronso0
Copy link
Member Author

ronso0 commented May 11, 2021

just realized your comments apply to main...puh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2.3 release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants