Syncplay / syncplay

Client/server to synchronize media playback on mpv/VLC/MPC-HC/MPC-BE on many computers
http://syncplay.pl/
Apache License 2.0
2.1k stars 214 forks source link

Minimizing and Restoring Syncplay Window on macOS? #178

Closed blaenk closed 6 years ago

blaenk commented 6 years ago

I've guided someone to use Syncplay on macOS and everything has worked out fine, except that it appears that minimizing and subsequently restoring may not be working properly on macOS.

I'm not positive on this, as this person isn't really technically minded, but the way they described it to me it seemed like after minimizing, they couldn't find and restore the window. I don't currently use macOS but I asked if they could click on the dock icon to bring it back, and they couldn't. Then I instructed them to right click on the icon and click on "Show all windows" and they said that it didn't show up then, but after that "effect", the window reappeared.

Can someone confirm that minimizing the Syncplay window and subsequently restoring it works correctly on macOS? It may have just been a misunderstanding on their part.

AFAIK everything else works perfectly.

Client: 1.5.1 OS: 10.13 High Sierra

albertosottile commented 6 years ago

I just tested this on my High Sierra machine. Syncplay typically shows two windows: the configuration window (the one that opens with the app) and the main window, which opens after the settings have been confirmed. The configuration window cannot be minimized since the corresponding button is disabled. The main window can be minimized and brought back without any issue.

However, the configuration window does not show on Mission Control, because is a modal window. So, maybe your friend just triggered mission control and went back. Then the window "disappeared" simply because it went behind another window from a different program. In this case, clicking on "Show All Windows" does not show anything. The only way to bring the configuration dialog back is to click once on the Syncplay icon in the dock, like you did.

Since this happens with every modal window in Syncplay (file dialogs, about, settings, etc...) I always thought this was their intended behavior and not a bug. After all, with the other dialogs, the main windows is always shown on Mission Control, so the user is never confused. However, I realize that with the configuration dialog, the user could be left with no windows from Syncplay at all.

@Et0h Maybe we could set the configurationGUI dialog to modeless, and see what this means on the other operative systems. EDIT: this is not that easy as I thought because at the moment Syncplay waits for return from the configuration modal window before opening the main window. If the ConfigDialog is set to modeless, the two windows open together at the start of Syncplay.

blaenk commented 6 years ago

I should have been more specific, but I was referring to the main window (after the configuration window), although perhaps they were confused and in fact they were referring to the configuration window.

Feel free to close this. If it happens again I'll try to get more information and report back, but it's possible that they may have just been confused.

Et0h commented 6 years ago

@albertosottile The configuration window shows up in the Taskbar for me on Windows 10. Would an easy compromise for macOS to be to use some sort of 'always on top' option for the Syncplay config window to prevent it from getting hidden. It is something like adding Qt.WindowStaysOnTopHint to line 1276 of GuiConfiguration.py.

albertosottile commented 6 years ago

@Et0h This could work, although I had to put the flag right before the exec_ method was called, otherwise the flag was reset somewhere else (found this here: https://stackoverflow.com/questions/19097323/setwindowflagsqtwindowstaysontophint-hides-qt-window ). The configuration dialog still disappears when Mission Control is triggered, but comes back always on top of all the others when the Desktop is brought back.

Should we apply this to syncplay/master too? Do you want to trigger this behavior only on macOS or an all operative systems?

albertosottile commented 6 years ago

Somewhat off topic: I wonder why the "Don't always show the Syncplay configuration window" is not toggled by default after the first successful configuration.

Et0h commented 6 years ago

Should we apply this to syncplay/master too? Do you want to trigger this behavior only on macOS or an all operative systems

Short answer: I can't see why we shouldn't apply it to syncplay/master for macOS, but I think for now it is best to limit the change to just macOS.

Long answer: The case for change on macOS is clear as it addresses a problem that users have reported. I do not have a strong view on which behaviour would be better for me as a user for other OS, but as a developer my inclination would be to maintain the current behaviour (i.e. not always on top) for other operating systems as that is the least likely to cause new problems. If someone makes a strong case to change the behaviour and addresses the potential concerns then I would be happy to give it some more thought.

I can see why some people might want it in front so they can't lose it while others might prefer it to not be always on top. People might end up looking at their browser or messaging program while launching Syncplay to copy over server and room information, but whether that would make them want it to be on top or not might depend on the user and their set-up. The main problem I can currently foresee with changing the behaviour is in circumstances where people have a low screen resolution as this would mean that the 'Always on Top' Syncplay window would take up a large proportion of the screen and make it hard for them to copy stuff over. We could make it not 'always on top' by default but allow people to enable this through a change to syncplay.ini but if nobody is that bothered that may just be added complexity for no good reason.

So, as per the above, the relevant principles here seem to be the desires for consistency, continuity, compatibility, usability, simplicity and functionality and these principles either point in opposite directions or in no clear direction at all. If someone can make a good argument in favour of a change to the status quo that considers the concerns raised about potential adverse implications then I am all ears, but until then it seems that the best course of action is to make it a macOS-only change.

Somewhat off topic: I wonder why the "Don't always show the Syncplay configuration window" is not toggled by default after the first successful configuration.

Always good to wonder about why things are as they are. This behaviour has had a number of evolutions over the years, and its precise working could very well change again in terms of handling how Syncplay works when you open a file with Syncplay. Some of the historic decisions were made based on an assumption that people would typically open Syncplay by using the 'open with' option on the media file they wished to play, which would make launching Syncplay.exe on its own the exception for when they wanted to configure some options first. This made a lot of sense back before the shared playlist feature because the user had to manually choose which video to play rather than being able to rely on the playlist to load it for them.

In terms of why I currently believe that Syncplay should continue to have a bias in favour of the configuration window I have a few arguments:

  1. There is a reasonable chance that someone will want to change their configuration options from time to time or perhaps even every time. For example, they might sometimes want to change which server they connect to, or the password might change, or they might want to join a different room by default, or they want to change one of the advanced configuration options.

  2. It can be better for users to see the options in advance so that they can double check them as the alternative is that they might accidentally join the wrong room on the wrong server by mistake then spend half an hour waiting for someone in the wrong place! If you change it a lot, it is good to be able to check every time, and if you don't change it often then you might forget to check that one time you did change it.

  3. There is currently no other obvious way to easily launch the configuration window - yes there is a command line option, but people might not know or they might forget about it. We could come up with a separate "Run Syncplay config" shortcut, but that would take up more desktop space and people would have to not forget about it if it is only accessible in their start menu. We could make it possible to change various options after you run Syncplay, but that'd need a lot of code changes.

blaenk commented 6 years ago

Yeah, for what it's worth, that's why I specifically instructed my non-technically inclined friends (and even those technically inclined) to not change that setting, so that the configuration always shows up in case any details ever need to be changed, since it isn't obvious how to relaunch it, and would be a hassle to guide them each through that process.

Maybe if the main Syncplay window had a menu entry that launched it, or if that's too much trouble, a menu toggle to "Launch Configuration on Next Launch."

For us though, it's no hassle to just launch the configuration window each time.

albertosottile commented 6 years ago

@Et0h I understand your concerns and I agree that there are good reasons to show the ConfigDialog every time. However, I am not in favor of the "always on top" solution, for the reasons you mentioned and also because it looks quite ugly. I believe that the right solution would be to transform the ConfigDialog in a modeless dialog. In this way, the dialog will never "disappear" and it will behave as expected in the macOS environment. Also, in my humble opinion, modal dialogs should be used only for file dialogs, or option dialogs, anything that requires an immediate and unskippable action by the user.

I implemented this change in my fork by creating a QEventLoop right before launching the dialog.show() method and terminating it with a custom signal when the dialog is closed (by closeEvent, resetSettings, and _saveDataAndLeave). As far as I know, nothing is returned when the ConfigDialog is closed (so nothing is lost if we use show() instead of exec_() ). Also, no other methods in ConfigDialog are triggered when the dialog is closed in any way. If I am wrong, both these issues can be fixed by adjusting the code accordingly.

I performed some tests on macOS and everything works. The ConfigDialog is correctly shown in Mission Control and also when all the macOS-specific display features are used.

What is your opinion about this solution? Do you think that it should be restricted to macOS only? If so, it can be pushed right away, otherwise it would probably be better to open a PR.

Et0h commented 6 years ago

@blaenk Ever since Syncplay 1.2.6 (released in June 2013) the "Don't always show the Syncplay configuration window" option has only affected how Syncplay behaves if you directly open a file with Syncplay. If you open Syncplay by itself without a file it will always open the configuration window irrespective of the option. The behaviour is explained in http://syncplay.pl/guide/client/ as follows: Don’t always show the Syncplay configuration window – Means the configuration window is not shown when opening a file with Syncplay (unless required configuration options need entering).

@albertosottile I'll test out your fork to see if it is fine on Windows when I have time. I don't have a firm view as to whether it is best to have it macOS only or be for all OS, but my inclination is that in the interests of long-term code maintainability the fewer OS divergences the better so if it can be done in the same way on every platform it makes things simpler for if we ever want to tweak the behaviour of how the config window operates.

blaenk commented 6 years ago

Good to know 👍 I think that's the most desirable behavior.

Et0h commented 6 years ago

I ran a test of 7fe1e942f4e77e0b8fdc2c22e254fc934aef111d on Windows 10 with the following activities and all behaviour was as expected:

  1. Run as normal
  2. Select 'store config and run' but specify no hostname to force things back to the config window
  3. Select 'restore defaults'
  4. Run with invalid password
  5. Run and closed

For the 'run with invalid password' test case it runs and closes with message box as expected, but in the future we might want to bring people back to the config dialogue with the wrong password error if that can be done easily.

albertosottile commented 6 years ago

Fixed by changing the modality of ConfigDialog. We will close this as soon as 1.5.2 is released.

blaenk commented 6 years ago

1.5.2 has been released.

If this was prematurely closed feel free to reopen.