Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance enhancing and fix resizing issues #1263

Closed
wants to merge 6 commits into from
Closed

Performance enhancing and fix resizing issues #1263

wants to merge 6 commits into from

Conversation

MuhammadMuradG
Copy link
Contributor

@MuhammadMuradG MuhammadMuradG commented May 21, 2021

Describe your changes in detail

This PR provides an enhancement in Box widget performance by using already calculated properties width and height from layout class, also this enhancement will fix issue #1205; by preventing the widgets size to be recalculated after window resizing.

What problem does this change solve?

This will solve the issue of limiting resizing of window after it be big.

This will solve

#1205

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

MuhammadMouradG and others added 6 commits May 21, 2021 17:09
There is no needing to add `padding` because the padding is calculated already in `self.interface.layout.height`
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I'm seeing the fix that you're claiming here.

Using current master (0447a36), I'm not seeing any significant change in behavior before/after this patch using the tutorial3 and multilinetextinput demos.

I do, however, see a change in tutorial1 - the window size is no longer constrained to the size of the widgets in the layout; only to the size of the "calculate" button.

@MuhammadMuradG
Copy link
Contributor Author

@freakboy3742 I attached two videos to illustrate the issue. The proposed patch is applied to current master (183493a)

Without the Patch

withoutpatch

With the Patch

withpatch

@MuhammadMuradG
Copy link
Contributor Author

MuhammadMuradG commented May 24, 2021

I do, however, see a change in tutorial1 - the window size is no longer constrained to the size of the widgets in the layout; only to the size of the "calculate" button.

@freakboy3742 I would also to pointing to the following, this behavior that you are encounter, I am encountering it from time to time without this patch especially in the first time that I am starting run the application, I don't know exactly what its cause, but what I can claim now and as an initial thought, it is because latency in loading of some python files, especially if we run in VM. However, for me this problem disappear if I restarted the toga application.

@freakboy3742
Copy link
Member

On current master (c563450), this is what I see with tutorial 3:

webview.mov

In your own recording, the window size doesn't reduce when made smaller from the original size. There's nothing in the app code that constrains the size of the window; so the only "True minimum" is the minimum widget sizes (which will be ~200x200). So - your "fixed" behavior isn't correct.

As for the behaviour in tutorial 1 - that can't be explained by "latency in loading python files" - the code is either loaded or it isn't. However, the choice to not re-evaluate the size of widgets on a window resize will affect the width of the text inputs - and thus the reflow size of those widgets in the layout. To the extent your patch "fixes" the problem, it again constraints the minimum size of the window to the original size of the window, which, once again, is a constraint that doesn't exist in the app code.

@MuhammadMuradG
Copy link
Contributor Author

Well, I reinstalled ubuntu 20.04 and run tutorial 3 from current master (c563450) of beeware/toga but what I encountered is unlike what you showed in your video; I am unable to minimize the window smaller than its initial size (640, 480). Also, I think what I have is consistent with what code does. Following is the illustration:
If you add the following line to the toga.Box widget:

    if min_width > width:
        width = min_width

+   print(min_width, width)

    return min_width, width

and then run tutorial 3 you will see that the minimum width (min_width) is equal to the the window size and then you will not be able to shrink the window less than this size. So, as I expect the current implementation is not desired; because it treats the initial window size (640, 480) as a minimum size of window. So, we now need to look at the do_get_preferred_width() method to fix this behavior. Please, @freakboy3742, confirm this if you agree...

@freakboy3742
Copy link
Member

Ok - so there's clearly something else going on here. I'm guessing the initial width is being evaluated as small on my screen, but large on your screen, explaining the min_width discrepancy. The complex interaction of Linux, XWindows, GTK and the desktop theme and more means you and I are seeing slightly different behavior.

At the end of the day, the code in the demo imposes no size limits - therefore, the widgets should be able to reduce in size. There isn't even an explicit initial size of the browser window. Therefore, the window should be able to reduce in size (and this is the behavior we see on other platforms). If your window can't reduce in size using current master, then that is a bug.

@MuhammadMuradG
Copy link
Contributor Author

I will close this and open new one that addressing the min size bug in gtk backend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants