Next one... ;)

Nice getting a bug fixed I added to the pile ;)

I'll look at it later, for now I'm only wondering why it already works for Browse mode but not for View. Perhaps add to the summary where the underlying reason is or whether you just duplicate the approach from there.

Could someone check for problems in a multi monitor setup?

In VirtualBox you can add more virtual screens for testing. (I'll do that in a bit.)

In D10248#199516, @rkflx wrote:

Could someone check for problems in a multi monitor setup?

In VirtualBox you can add more virtual screens for testing. (I'll do that in a bit.)

Thanks for the tip. :)
As I feared mapToGlobal() counts including all screens, but mapTo() does it.
Is there any way to stretch gwenview's fullscreen over multiple screens?

What to do with Full Screen mode with multiple displays is actually a more general issue for all software, but it seems that the optimal solution is task-specific. E.g. it doesn't make sense for a video player or a text editor to take over all screens, but maybe it does for Gwenview?

In D10248#199552, @ngraham wrote:

What to do with Full Screen mode with multiple displays is actually a more general issue for all software, but it seems that the optimal solution is task-specific. E.g. it doesn't make sense for a video player or a text editor to take over all screens, but maybe it does for Gwenview?

Maximizing or going fullscreen apparently applies only to one monitor at a time, if you want Gwenview spanning multiple displays you'd have to use the windowed mode. I wonder how people using huge display walls or frameless displays will view images in such scenarios? IMO that includes playing videos! Are they supposed to use special software for that? Should Gwenview support that? How is GNOME / MacOS / Windows handling this?

If anyone could answer that, a bug report / wish to add that as a non-default option might be worth a shot.

In D10248#199543, @muhlenpfordt wrote:

Changed screen mapping to work on a second display

Good thing we checked that, there were serious problems indeed. FWIW a screenshot I took right before you patched it:

Now it's working fine.

In view mode a custom widget is used, in browse mode a standard KToolBar where this problem does not appear

That's what I suspected. However: Could we fix the custom widget to properly react to resize events by itself? Doing it from the outside with EventWatcher::install seems quite brittle and unusual to me. At least for Browse it's working, otherwise you wouldn't need the extra check.

However: Could we fix the custom widget to properly react to resize events by itself? Doing it from the outside with EventWatcher::install seems quite brittle and unusual to me. At least for Browse it's working, otherwise you wouldn't need the extra check.

That was my first thought, too ;) - but the widget is not resized, just moved. Even move events only appear while sliding FullScreenBar in/out.
Tried an EventWatcher on mContent to get resize events in FullScreenContent, but ViewMainPage seems to be the only widget that gets the correct events.
I could call the EventWatcher::install() inside FullScreenContent::init() for watching resize events of autoHideParentWidget. This appears cleaner, but in reality this widget is ViewMainPage.

I guess the main problem is the autohide feature of this toolbar and therefore it cannot be added to a layout that will automatically resize the widget.

Forgot something:

In D10248#200363, @rkflx wrote:

As far as my understanding goes after investigating, FullScreenBar does not get resizeEvents, while ViewMainPage does. While the latter is set as a parent of the former, that's not enough to propagate layout events: You'd need to add FullscreenBar to a layout which itself is attached to ViewMainPage.

Maybe it would be helpful to add that to the summary as well.

In D10248#200363, @rkflx wrote:

Nevertheless, have you tried this? I don't think the feature is the problem. It's the implementation of the feature which might make it difficult. Would be interesting to know whether adding a layout works (it certainly would be less hackish).

I tried inserting the FullScreenBar to ViewMainPage's layout. This way the resize events come through and the bar automatically adjusts its size - but the image is not full screen height anymore, since the toolbar always takes the space above. I think we would need a custom layout for an overlapping widget, something like a modified QStackedLayout. Not sure, if this much better. ;)
If you agree with the latest modifications I'll commit it this way?

Idea for a small follow-up fix (squished buttons on the left):

This only happens in fullscreen mode by moving the splitter to the right?

In D10248#201161, @muhlenpfordt wrote:

I tried inserting the FullScreenBar to ViewMainPage's layout. This way the resize events come through and the bar automatically adjusts its size - but the image is not full screen height anymore, since the toolbar always takes the space above. I think we would need a custom layout for an overlapping widget, something like a modified QStackedLayout. Not sure, if this much better. ;)

Okay, makes sense. Let's go with your approach instead of making it even more complicated.

If you agree with the latest modifications I'll commit it this way?

Sure, go ahead.

Idea for a small follow-up fix (squished buttons on the left):

This only happens in fullscreen mode by moving the splitter to the right?

That's correct, I increased the width of the sidebar.

In D10248#200363, @rkflx wrote:

For now, I'm accepting the patch. If you are fast with committing, it might make it into 17.12.2.

I should have been more precise with that: I meant "commit to stable". Or is there any reason it should go to master only? If not, you could cherry-pick the fix from master to stable.

In D10248#201184, @rkflx wrote:

I should have been more precise with that: I meant "commit to stable". Or is there any reason it should go to master only? If not, you could cherry-pick the fix from master to stable.

Seems I missed that hint and simply did not thought of commiting to stable.

That was easier than first pushing to stable. :)

$ git co 'Applications/17.12'
$ git cherry-pick 613dde719578427a41c34c85118fc628c329791d
$ arc land --hold --onto 'Applications/17.12'
$ git push -- origin b3e66abe28c2a85f0891f9212403ff6648a8e2eb:Applications/17.12

I just realized that I created two different commit hashes in master and stable. Will that cause problems in future?

In D10248#201597, @muhlenpfordt wrote:

I just realized that I created two different commit hashes in master and stable. Will that cause problems in future?

Not at all, the opposite would be something to be concerned about. Just research a bit about commit hashes and you'll see why that is.

In D10248#201598, @rkflx wrote:

Not at all, the opposite would be something to be concerned about. Just research a bit about commit hashes and you'll see why that is.

Of course a new commit must have a different hash. I just wondered why it's only one commit in 74509b4af000, but I think I got it now.
I was a bit confused and thought of merging master and stable when a new stable is created, but that is nonsense, it's just a new branch from master.
Ok, forget my silly question... ;)

In D10248#201769, @muhlenpfordt wrote:

Ok, forget my silly question... ;)

Not at all, just ask away if you have questions. We don't want to break the repo, after all ;)

Maybe you were confused what will happen once we add another bugfix to stable and then merge the stable branch to master again: Nothing, because Git will see that the changes are already done in both branches, even though they have different commit hashes.