Reintroduce aspect ratio logic for tiling containers #3810

orestisfl · 2019-10-01T13:29:58Z

xzfc · 2019-10-03T05:02:20Z

After this commit, mpv aspect ratio hints are honored as well (unlike 4.16).

Ignoring mpv aspect ratio hints was a useful feature: mpv could display OSD outside of the movie frame. And this was consistent with i3 behavior regarding "resize increment" hints for tiled windows.

I've checked it using slop -b 20 across commits:

commit	mpv	mplayer
`29f2510`^	ignored	honored, no bug
`29f2510`	ignored	ignored, bug
`6b8b3dc`^	ignored	ignored, bug
`6b8b3dc`	honored	honored¹, no bug

Aspect ratio hints are ignored right after mplayer startup but honored after workspace switching.

orestisfl · 2019-10-03T09:20:06Z

Thank you very much! I'll investigate.

Before 29f2510, tiling windows confronted to their aspect ratio and the windows were centered in their container. This commit re-introduces this logic which is implemented differently than the logic in floating_check_size. Fixes i3#3795

orestisfl · 2019-10-03T13:18:47Z

I think that the problem with mpv is that before 29f2510 i3 did not save the aspect ratio normal hints in manage_window so mpv had window->aspect_ratio == 0.0.

I don't think there is any reasonable way to correct that. I much prefer to not introduce problems to the more popular mpv player in order to fix a bug in mplayer.

@Airblader any other ideas?

Airblader · 2019-10-05T22:16:04Z

In my understanding the bug lies with mplayer, right? As such I wouldn't try to work around its issues in i3, particularly if it causes issues with other clients (more popular or not).

orestisfl · 2019-10-05T23:00:00Z

In this case, I don't know which is the "correct" behavior. i3 exposes a bug in mplayer but it would be possible for i3 to misbehave according to the standard.

According to the comment by Michael:

i3/src/render.c

Lines 67 to 75 in f397698

    
                   /* Obey the aspect ratio, if any, unless we are in fullscreen mode. 
        
                    * 
        
                    * The spec isn’t explicit on whether the aspect ratio hints should be 
        
                    * respected during fullscreen mode. Other WMs such as Openbox don’t do 
        
                    * that, and this post suggests that this is the correct way to do it: 
        
                    * https://mail.gnome.org/archives/wm-spec-list/2003-May/msg00007.html 
        
                    * 
        
                    * Ignoring aspect ratio during fullscreen was necessary to fix MPlayer 
        
                    * subtitle rendering, see https://bugs.i3wm.org/594 */

the spec is not clear about fullscreen windows (and certainly not about "tiling" windows) but in this specific case I feel that we should ignore the aspect ratio hints (as in 4.17) like we do for the rest of the normal hints.

xsawyerx · 2020-06-08T10:20:05Z

I'm suffering from the issue this PR is fixing. I was wondering if there are plans to continue the work and merge this fix or a different one?

orestisfl · 2020-06-08T10:45:43Z

I will gladly make the changes if we reach an agreement. @stapelberg do you have an opinion on this?

…

On Mon, Jun 8, 2020, 12:20 Sawyer X ***@***.***> wrote: I'm suffering from the issue this PR is fixing. I was wondering if there are plans to continue the work and merge this fix or a different one? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3810 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABMCZPWAP63HEJONHQHHTWTRVS3OJANCNFSM4I4JQOLA> .

stapelberg · 2020-06-10T07:56:35Z

Could you summarize what the question is please? I have read this issue, but it’s not 100% clear to me.

In particular, #3810 (comment) says that with your latest commit, both mpv and mplayer do work as expected (isn’t that the desired outcome?), or am I reading this wrong?

PS: Hey @xsawyerx! I remember reading your name many times when I was working with Perl. Thanks for all your work, and enjoy i3! :)

xsawyerx · 2020-06-10T08:42:39Z

Could you summarize what the question is please? I have read this issue, but it’s not 100% clear to me.

I don't know if I'm phrasing it well, but, when using mplayer, the window area, past the video border itself, does not stay black. Instead it reflects other windows backgrounds (like the workspace I moved from, the last image on the screen in full screen, etc.).

In particular, #3810 (comment) says that with your latest commit, both mpv and mplayer do work as expected (isn’t that the desired outcome?), or am I reading this wrong?

I had missed that. I'm certain I'm not using the latest version, so I'll first upgrade and then check again. Apologies if I added more noise to a resolved issue.

PS: Hey @xsawyerx! I remember reading your name many times when I was working with Perl. Thanks for all your work, and enjoy i3! :)

I've been a very happy i3 user for a long time now. I think it is I who owes you a thank you. :)

xsawyerx · 2020-06-10T08:47:58Z

In particular, #3810 (comment) says that with your latest commit, both mpv and mplayer do work as expected (isn’t that the desired outcome?), or am I reading this wrong?

I think this commit is not merged, so I'm not sure upgrading from 4.17.1 (current stable Ubuntu) to 4.18.1 (official stable) will fix the issue. Maybe I'm missing something.

stapelberg · 2020-06-10T09:17:13Z

I think this commit is not merged, so I'm not sure upgrading from 4.17.1 (current stable Ubuntu) to 4.18.1 (official stable) will fix the issue. Maybe I'm missing something.

Correct, the commit in question is pending in this very PR.

If you want to test, check out this PR and build i3 yourself. You can use apt build-dep i3-wm to get all the deps, and mkdir -p build && cd build && meson .. && ninja to build i3 :)

orestisfl · 2020-06-10T09:17:43Z

Could you summarize what the question is please? I have read this issue, but it’s not 100% clear to me.

In particular, #3810 (comment) says that with your latest commit, both mpv and mplayer do work as expected (isn’t that the desired outcome?), or am I reading this wrong?

PS: Hey @xsawyerx! I remember reading your name many times when I was working with Perl. Thanks for all your work, and enjoy i3! :)

"Honored" for mpv is not the optimal behavior as explained because subtitles can be placed in the dark areas that appear when the aspect ratios are not honored. I wonder if mpv should report different aspect ratios here because it clearly does not need/want to be that restricted.

stapelberg · 2020-06-17T15:31:46Z

"Honored" for mpv is not the optimal behavior as explained because subtitles can be placed in the dark areas that appear when the aspect ratios are not honored. I wonder if mpv should report different aspect ratios here because it clearly does not need/want to be that restricted.

Okay, so it’s not optimal because a welcome side-effect of ignoring the aspect ratio hints is no longer triggered. But, it fixes behavior otherwise. Did I get this right?

I’m okay with going ahead with the fix, and requiring people to take up mpv usability improvements with mpv upstream :). Given that mpv is an active open source project, we can fix this the right way instead of having to accept a workaround.

orestisfl · 2020-06-18T13:05:34Z

Ok, I will contact the mpv upstream

133-size-hints.t: Introduce cmp_ar shortcut

5249fdb

orestisfl changed the title ~~Aspect ratio~~ Reintroduce aspect ratio logic for tiling containers Oct 1, 2019

render.c: Reintroduce aspect ratio logic

bf5365f

Before 29f2510, tiling windows confronted to their aspect ratio and the windows were centered in their container. This commit re-introduces this logic which is implemented differently than the logic in floating_check_size. Fixes i3#3795

orestisfl force-pushed the aspect-ratio branch from 6b8b3dc to bf5365f Compare October 3, 2019 10:45

orestisfl added the discussion label Oct 12, 2019

orestisfl added the waiting Waiting for feedback/changes from the author of the issue/PR. Make sure to notify us with a comment. label Jun 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintroduce aspect ratio logic for tiling containers #3810

Reintroduce aspect ratio logic for tiling containers #3810

orestisfl commented Oct 1, 2019

xzfc commented Oct 3, 2019 •

edited

orestisfl commented Oct 3, 2019

orestisfl commented Oct 3, 2019

Airblader commented Oct 5, 2019

orestisfl commented Oct 5, 2019 •

edited

xsawyerx commented Jun 8, 2020

orestisfl commented Jun 8, 2020 via email

stapelberg commented Jun 10, 2020

xsawyerx commented Jun 10, 2020

xsawyerx commented Jun 10, 2020

stapelberg commented Jun 10, 2020

orestisfl commented Jun 10, 2020

stapelberg commented Jun 17, 2020

orestisfl commented Jun 18, 2020

Reintroduce aspect ratio logic for tiling containers #3810

Are you sure you want to change the base?

Reintroduce aspect ratio logic for tiling containers #3810

Conversation

orestisfl commented Oct 1, 2019

xzfc commented Oct 3, 2019 • edited

orestisfl commented Oct 3, 2019

orestisfl commented Oct 3, 2019

Airblader commented Oct 5, 2019

orestisfl commented Oct 5, 2019 • edited

xsawyerx commented Jun 8, 2020

orestisfl commented Jun 8, 2020 via email

stapelberg commented Jun 10, 2020

xsawyerx commented Jun 10, 2020

xsawyerx commented Jun 10, 2020

stapelberg commented Jun 10, 2020

orestisfl commented Jun 10, 2020

stapelberg commented Jun 17, 2020

orestisfl commented Jun 18, 2020

xzfc commented Oct 3, 2019 •

edited

orestisfl commented Oct 5, 2019 •

edited