FvwmPager: Improve window preview position calculations.

[topcat001]

• Explicitly check for zero sizes and adjust only then.
• While computing x and y, subtract 1 from the sizes before scaling.

[ThomasAdam]

Thanks, @topcat001 -- all looks good to me. I'd like @somiaj to just glance at this to check everything is still OK with the maths. But I don't see anything amiss here.

[somiaj]

I'm still trying to understand fully what is going on here, but it appears that you are just forcing all rounding errors to be rounded down to avoid an over computation. I'm unsure if this is the best way in the long run, but it will work. My initial thought was to snap to page edges, but rounding down is fine.

I also unsure why you need to change adjust_for_sizehints. This just changes how the overall pager window size is computed to make all pages the same size. So I don't think those changes are needed and we don't gain much to add more logic in computing the pager windows height/width, even if it is slightly more accurate. The current method was done for simplicity. Your changes are fine too.

I haven't tested this patch out, but just looking at the code, and these are my initial thoughts.

[topcat001]

Thanks @somiaj. Yes rounding down seems to work quite well, although it is not ideal. While debugging, I found that adjust_for_sizehints is overestimating the sizes scaled_height and scaled_width (due to unconditionally adding +1) to be often greater than the pager widow area, which results in the scaled widow sizes to be too large later (even with my rounding down changes) in fvwmrec_to_pager, so much so that the final (cumulative) error is often more than just one pixel. Hope that makes sense. So, both changes were necessary to get the window previews to render without clipping (when the windows are not actually off the screen).

In all these tests I am doing precise pixel level measurements of the rendered windows and comparing with the numbers in the size variables to determine the errors.

[ThomasAdam]

Thanks @somiaj and @topcat001 -- what's the verdict here? I say we at least merge this and give it a go and see how it performs in the wild. It's certainly better than it was, and if we need to tweak it before the next release, we can.

[topcat001]

Thanks! I haven’t had a chance to explore snapping yet but I’ll play with it later to see if it improves things.

---

From: Thomas Adam @.> Sent: Tuesday, August 2, 2022 6:09:15 AM To: fvwmorg/fvwm3 @.> Cc: Anindya Mukherjee @.>; Mention @.> Subject: Re: [fvwmorg/fvwm3] FvwmPager: Improve window preview position calculations. (PR #679)

Thanks @somiajhttps://github.com/somiaj and @topcat001https://github.com/topcat001 -- what's the verdict here? I say we at least merge this and give it a go and see how it performs in the wild. It's certainly better than it was, and if we need to tweak it before the next release, we can.

— Reply to this email directly, view it on GitHubhttps://github.com/fvwmorg/fvwm3/pull/679#issuecomment-1202516067, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AFLGE6R4FPOF522FBQ74KJLVXEMXXANCNFSM54S6TSQA. You are receiving this because you were mentioned.Message ID: @.***>

[somiaj]

I think in principal this is a fine approach, the math seems to fudge things a bit in multiple places, so in terms of that it maybe harder in the future to understand the reason behind the fudging, but in the end the problem is converting reals to integers, which there is no best way. Snapping to me would be cleaner, but this works.

I would suggest the code cleanups I mentioned above, as there is some logic that just isn't needed.

Since the page is just an approximate anyways, this is a fine approximate if it works. I'll compile and test it out later.

[ThomasAdam]

Thanks, @somiaj - @topcat001 can you make the changes @somiaj is suggesting and I'll approve this PR.

[somiaj]

I just tested it with my suggested changes and everything seems to work fine and windows are no longer too big and the bottom/right borders no longer go over the page boundaries.

[topcat001]

Hi @somiaj I followed your suggestion to roll back changes in adjust_for_sizehints and only kept the changes in fvwm_rec_to_pager. Unfortunately in my setup this does not fix the problem. windows are still being clipped. It's because the pager window is computed to be bigger than the display area. This causes even the updated window size computation to be to0 big in fvwm_rec_to_pager. I have a single 1080 screen, not hight dpi, and the default config.

My pager window is quite small (default config) which makes the problem more apparent probably.

[somiaj]

In further testing I found that your changes to adjust_for_sizehints has unexpected visual effect. In some cases it makes the size of each page too small, causing a border of the pager to appear (see the following screenshot). So though this can fix one visual bug, it seems it can cause another. I cannot get this to happen unless I apply your changes to adjust_for_sizehints.

Notice the white line on the bottom/right border (you might need to click on the image to see it full sized).

As to your last comment, the change there is needed to make your change fix the original visual bug, but it adds a new visual bug for some sizes of the pager.

[topcat001]

Thanks @somiaj I confirmed your observation. Ok, let's roll that back as even without that one the current update makes things slightly better. I think I'll also add a snap update later with the corrected sizes, but will need to play with it more. I like the snapping idea but haven't properly explored it yet.

[topcat001]

@ThomasAdam I believe the code is now updated according to @somiaj's recommendations.

At a later point I will add a snapping feature. The size and position updates in the current diff are still needed I think because I found that windows are too big and too far down and to the right (because to x and y being too big). However an additional snapping (where I move the more accurately sized rectangle to snap to the edge) will fix any remaining visual issues.

[somiaj]

@topcat001 you didn't apply any of my other suggested changes int he code review due to extra unneeded logic. Also you should probably just squish all these commits into a single commit, then force-update your branch.

[topcat001]

@somiaj sorry could you post the diff which you used in your local tests? I might have missed something. Thanks!

[somiaj]

@topcat001 look up or at the 'files changed' tab and you'll see comments I made next to the code.

[topcat001]

@somiaj really sorry don't know if it is my Chrome but I can't seem to see any comments in the files tab? I can only see your comment about adjust_for_sizehints changes. Not sure what is the issue. Again, my apologies.

[somiaj]

I'll try to clean up a better fix, but will be out of town for the next few days, so I'll look at it when I get back. I have some ideas, they just didn't work with my initial testing and I don't want to debug them at the moment.

[ThomasAdam]

Hey both -- has there been any further thoughts on this? I've not really paid much attention to IRC. Just in case there might have been some discussion.

[topcat001]

Hi, we haven't discussed this since the last comment.

[somiaj]

Replaced by #756.