Waitsnake / AnimatedGif

A screensaver for Mac OSX / macOS that plays animated GIFs and APNGs
MIT License
213 stars 31 forks source link

[10.13.4] Support multiple screens in background mode #33

Closed ramiro314 closed 6 years ago

ramiro314 commented 6 years ago

Right now the background mode only supports the mainScreen ( https://github.com/Waitsnake/AnimatedGif/blob/master/AnimatedGif/AnimatedGifView.m#L113 ). It would be nice to support other connected screens as well.

It might be a stretch, but it would also be great to be able to pick what gif will be shown in what screen.

Waitsnake commented 6 years ago

Well the code line you marked out at least shows that when I do the workaround of issue #29 I only had one screen in mind.

I played a bit with the issue and tried different code changes in an multiple screen setup and analyse the problem a bit deeper. The real problem that I found is that the ScreensaverEngine of macOS, that runs the ScreenSaverView (the class AnimatedGif inherits from), only supports multiple screens when it is running in normal screensaver modus. Than multiple screens working pretty much out of the box. Unfortunately in normal screensaver mode all the windows are running in an extra space that is different from the desktop space. There is no way, at least that I know, to use this normal screensaver mode and change it afterwards in a way that it runs in a background mode for the desktop.

When the ScreensaverEngine is startet with parameter "-background" (to let the ScreensaverEngine do job of the background mode for you; but this is not working under 10.13) or "-window" (this creates a normal window with an screensaver in it; this is the actual workaround for issue #29) it creates in both cases only one window with a screensaver for you independent from the number of screens you have. In other words without a multiple screen support from Apple for the options "-window" or "-background" you can pretty much put the hole class ScreenSaverView to the trash.

I will not say it is technically not possible to create later extra windows depending on the number of screens you have, but than you need to find a way the methods initWithFrame() and animateOneFrame() are called multiple times for each window/screen and that also all involved object attributes like self.bounds are switched for each window/screen instance before these methods are called. I could think of a new class that is between the original class ScreenSaverView and the existing screensaver code of AnimatedGif. That new class capsulate the problem and creates the needed windows and do the context switching between them and call the old screensaver code.

Well it sounds as it is: It is a bigger problem that needs more time and effort as it seems on the first look. Since there is no support of the ScreenSaverView class for background mode and multiple screens at the same time I don't see it as a bug of AnimatedGif and I put an enhancement label on it. But I'm not sure when I will find the time for this, but it is a good goal for the future of AnimatedGif anyway.

The second request you had with selecting different Gifs for each screen individually will probably never happen since there is also no support from operating system to do this in the preference pane individually for each screen (as Apple did it for selecting the wallpaper individually for each screen for example). I will not create an own preview window that shows all screens and let you decide witch Gif is for witch screen. Sorry this is way to much for my actual skills as macOS GUI programmer. But if you have better GUI programming skills under macOS you can fork the project and implement it the way you like it at any time.

ramiro314 commented 6 years ago

Understood, thank you for the detailed explanation. As you said it seems like this is caused mainly by the workaround. Before making you expend any time on this I'll check if Apple fixed the bug in the latest High Sierra release. Later today I'll try with AnimatedGif v1.3.3 (this seems to be the latest version before the workaround)

Thanks!

Waitsnake commented 6 years ago

Yes, with version 1.33, before the High Sierra workaround, the background mode is working under El Capitan with multiple screens. Thanks for your feedback.

It looks like, when I checked the "-background" option last time with version 1.36 I fool myself maybe due to other side effects of software changes since than (maybe both screensaver views were rendered at the same position on the same screen or something?). Seems I need to add more logging commands into the code next time I take a look at it.

But I'm sure that the "-window" option for the High Sierra workaround only creates one window even when having multiple screens. I tried the option without changing anything at the window properties and there was really only one window visible at the foreground that I could grab with the mouse and move around the desktop.

But whats the next step? Implement two different ways for the background mode of the screensaver for different versions of the operating system? Actually I would like to avoid that. And as far as I remember the "-background" option has other issues that the "-window" option don't have (like ScreensaverEngine crashes when coming back from sleeping) and so I already removed the workarounds (#14 and #15) for these problems. If I will support "-background" option again I need to bring back those workarounds too. Vise versa the "-window" option has a workaround #30 that the "-background" option don't need.

It would be a big hassle to deal with different behaviors of different operation systems in the screensaver with all there unique workarounds. So I wonder if I play the waiting game until Apple fixes the "-background" option for High Sierra?

By the way, what version of the macOS/OSX do you use?

Waitsnake commented 6 years ago

I digged a bit deeper into the problem with some more NSLog print outs.

In normal screensaver mode and with "-background" option the ScreenSaverEngine creates multiple instances for each screen of the ScreenSaverView class. But for the "-window" option it only creates one instance (!).

I don't saw the second instance on my second the screen when I tried it last time when using "-background" because the window level was set wrong and so the window of second screen was hidden below the desktop. The cause for this was this orderBack call here that changes the window level in a wrong way: https://github.com/Waitsnake/AnimatedGif/blob/master/AnimatedGif/AnimatedGifView.m#L180

That orderBack call came with 1.3.4 and thats why 1.3.3 still works with multiple screens. I used the orderOut and orderBack just to hide the black screen in the init phase of the screensaver. Well that problem of orderBack can be easily fixed by this call afterwards: [self.window setLevel:kCGDesktopWindowLevel];

So the "-background" option could be fixed on top of a 1.3.6 release. But there is still the question if I bring back that option anyway and support different operating system versions with different code? I'm still undecided with that since it would be only a fix for OSX versions below High Sierra.

And for the "-window" option the solution is not that simple since I know now for sure that Apples ScreenSaverEngine ignores multiple screens with this option and only creates one instance of the ScreenSaverView class.

Waitsnake commented 6 years ago

Well I found yet another workaround that uses the "-window" option for multiple displays.

Since on ScreenSaverEngine will only start on instance of ScreenSaverView class as I mentioned earlier I came up with the idea of starting multiple instances of the ScreenSaverEngine itself.

I get that running after a lot of testing, but this workaround works in a very round about indirect or complicated fashion. This means it could contain other side effects or problems that I'm at the moment not aware off.

I already found one of those side effects: "the change of display numbers while the background mode is running". I could fix that case by catching the system event for that and add some code to handle this case. But there could be still more of those cases. Thats why I hesitate the official release for some more testing.

Unfortunately I have only a two display setup (internal laptop screen and one attached display over mini-displayport/thunderbolt ) and I can't add more since I haven't a thunderbolt-splitter or a second display with mini-displayport/thunderbolt. Maybe you have a setup with more than two screens to test this out?

I could only test it under El Capitan (10.11) since I already restored my time machine backup and removed High Sierra from my computer a while ago. So I hope at least you can test this under High Sierra(10.13) if you have this version installed ?

I attach an unofficial prerelease of 1.3.7 just to this comment so that you can try it out if you want. AnimatedGif.saver.zip

ramiro314 commented 6 years ago

Hey, I'm on 10.13.4. I have a three monitor setup on my office. I'll test it out and tell you the results. Thanks for taking the time to work on this!

Waitsnake commented 6 years ago

Hi, have tried AnimatedGif with 10.13.4 ? I received reports that not even the normal screensaver mode works anymore, see issue #35 . Since you had with 10.13.4 only problems with multiple screens I guess there was maybe a minor update for 10.13.4 that makes thinks even worse?

Waitsnake commented 6 years ago

I could now check background mode with 10.13.4 by myself together with issue #35 but only for one screen. Under El Capitan checked it with two screens and it worked.

So I will release 1.3.7 now and if this issue here still makes problems with more than one screen we need to reopen this issue.