Morph >> openInWindowLabeled:inWorld: can open windows outside the usable area

[mrakgr]

Pharo 7.0.0 Build information: Pharo-7.0.0+rc1.build.128.sha.b66e0a4c5a7bc47ae4c3cec339f788bfdff0289e (64 Bit)

---

Motivated by the question of how to change the size of the System Browser I did some digging and found some peculiar things in the following method:

openInWindowLabeled: aString inWorld: aWorld
    "Changed to include the inset margin in the bound calculation."

    | window extent |
    window := (SystemWindow labelled: aString) model: nil.
    window 
        " guess at initial extent"
        bounds:  (RealEstateAgent initialFrameFor: window initialExtent: self fullBounds extent world: aWorld);
        addMorph: self frame: (0@0 extent: 1@1);
        updatePaneColors.
    " calculate extent after adding in case any size related attributes were changed.  Use
    fullBounds in order to trigger re-layout of layout morphs"
    extent := self fullBounds extent + 
            (window borderWidth@window labelHeight) + window borderWidth +
            (window class borderWidth * 2 @ (window class borderWidth + 1)). "include inset margin"
    window extent: extent.
    aWorld addMorph: window.
    window activate.
    aWorld startSteppingSubmorphsOf: window.

    window announceOpened.
    ^window

In RealEstateAgent initialFrameFor: window initialExtent: self fullBounds extent world: aWorld there will be various calculations of visible display areas and the window will be squished if it is oversized. Calling RealEstateAgent initialFrameFor: window initialExtent: self fullBounds extent world: aWorld does a perform to call the RealEstateAgent class >> cascadeFor:initialExtent:world:.

cascadeFor: aView initialExtent: initialExtent world: aWorld

    | position allowedArea |
    allowedArea := self maximumUsableAreaInWorld: aWorld.
    position := aWorld currentWindow isMorph 
        ifFalse: [ aWorld center - (initialExtent / 2)]
        ifTrue: [ aWorld currentWindow position + 20].
    ^ (position extent: initialExtent)
        translatedAndSquishedToBeWithin: allowedArea

This is why I think the windows are intended only to be open in the usable area.

extent := self fullBounds extent + 
        (window borderWidth@window labelHeight) + window borderWidth +
        (window class borderWidth * 2 @ (window class borderWidth + 1)). "include inset margin"
window extent: extent.

In this part, this is seemingly forgotten and the window will get opened regardless of where it is. Proposed change is to replace the above fragment with.

window bounds: (RealEstateAgent initialFrameFor: window initialExtent: window fullBounds extent world: aWorld).

Here is the full version for clarity.

openInWindowLabeled: aString inWorld: aWorld
    "Changed to include the inset margin in the bound calculation."

    | window |
    window := (SystemWindow labelled: aString) model: nil.
    window 
        " guess at initial extent"
        bounds: (RealEstateAgent initialFrameFor: window initialExtent: self fullBounds extent world: aWorld);
        addMorph: self frame: (0@0 extent: 1@1);
        updatePaneColors.
    " calculate extent after adding in case any size related attributes were changed.  Use
    fullBounds in order to trigger re-layout of layout morphs"
    window bounds: (RealEstateAgent initialFrameFor: window initialExtent: window fullBounds extent world: aWorld).
    aWorld addMorph: window.
    window activate.
    aWorld startSteppingSubmorphsOf: window.

    window announceOpened.
    ^window

---

I am very new to Pharo and with my overall grasp this kind of patch is really the best I can do. I only know that this fixes the issue of the System Browser being too large for the display, I am not sure if this will break things elsewhere.

As a side comment, I feel that setting the size of a window and then setting it again afterwards is convoluted. Moreover, I feel that recalculating the extent based on the border widths of the window should not be done in this method as was done in the original fragment. Calculating the extent should be the task of the class itself or the RealEstateAgent.

[bencoman]

Hi Marko, I'm not familiar with this part of Pharo (I like to play more in the system depths) but just wanted to welcome you to Pharo. Thanks for jumping in to help us improve it. When I was starting in Pharo I was often amazed at how easy it was dig into the depths. Hope you are enjoying it.

Morphic grew organically from lots of contributors over a number of years its not always clear the impact of changes. A change like this is probably best to go into Pharo 8. We are a growing but still a small community and with limited resources its useful to leave Pharo 7 fairly static cosmetically and just implement critical bugs. Indeed, policy is that all fixes need to go into Pharo 8 first and then critical ones back-ported to 7. Bleeding edge Pharo 8 is usually pretty stable and a lot of people prefer using it because its what the core developers use and you can get bugs fixed faster.

If you don't any help with it in a few days, try promoting in on the pharo-dev mail list. btw, do you have any screen snapshots showing the before/after difference?

[mrakgr]

Thanks for jumping in to help us improve it.

You're welcome.

Do you have any screen snapshots showing the before/after difference?

It should be easy to make some screenshots. Should I use imgur or something else?

Bleeding edge Pharo 8 is usually pretty stable and a lot of people prefer using it because its what the core developers use and you can get bugs fixed faster.

Ah, I see. A few weeks ago, I tried the latest development 7.0 version (in the Pharo) launcher and found that the System Browser is essentially broken in it so since then I've stuck to the template. I'll try getting the latest automated build for 8.0 on this repo.

One thing I've been wondering is how should updates be applied to the image? It seems me that the image is static once you load it and I haven't run into an explanation of how to do that yet.

[bencoman]

It should be easy to make some screenshots. Should I use imgur or something else?

You can just past directly into the issue on github. (also I believe the mail list will allow a screen shot size attachment)

Bleeding edge Pharo 8 is usually pretty stable and a lot of people prefer using it because its what the core developers use and you can get bugs fixed faster.

Ah, I see. A few weeks ago, I tried the latest development 7.0 version (in the Pharo) launcher and found that the System Browser is essentially broken in it so since then I've stuck to the template. I'll try getting the latest automated build for 8.0 on this repo.

One thing I've been wondering is how should updates be applied to the image? It seems me that the image is static once you load it and I haven't run into an explanation of how to do that yet.

To apply them locally to your own images? In the source image, right-click a class/protocol/method then Extra > FileOut. In the target image, Tools > File Browser > Install.

For official updates, it used to be these were done on a rolling basis, but that leads to poor reproducability, so Pharo had gone the path of Continuous Integration and produce a new build for each update.

[mrakgr]

To apply them locally to your own images?

I've meant how should my own image be synched with the bleeding edge that the core devs are using? I am hoping I do not need to start from a clean slate every few months. Right now I just got 8.0 and will have to import the packages that I'd made in the previous image by hand or from the Github repo. Since I only have a few this will not be a problem, but if I had many it would be.

At any rate, here are the screenshots. You will note how one of in one of the images the System Browser has sunk into the taskbar and below the visible area. This is from the 8.0 of Pharo that I just got so I can verify that the problem has not been fixed yet.

[bencoman]

On Wed, 13 Mar 2019 at 01:15, Marko Grdinić notifications@github.com wrote:

To apply them locally to your own images?

I've meant how should my own image be synched with the bleeding edge that the core devs are using? I am hoping I do not need to start from a clean slate every few months.

You should define a Baseline with all of your packages to make it easy to start from a clean slate with a single Metacello command.

Right now I just got 8.0 and will have to import the packages by hand or from the Github repo. Since I only have a few this will not be a problem, but if I had many it would be.

At any rate, here are the screenshots. You will note how one of in one of the images the System Browser has sunk into the taskbar and below the visible area. This is from the 8.0 of Pharo that I just got so I can verify that the problem has not been fixed yet.

Thanks for the snapshots but I don't yet understand what the problem is. Are you saying that if the window is too small, when the System Browser is opened it should shrunk to fit? What about if the Pharo window is just a quarter of a screen height and you then open a System Browser?

[mrakgr]

Are you saying that if the window is too small, when the System Browser is opened it should shrunk to fit?

Yeah, until I fixed this it was definitely annoying to have it sink. For longer methods, every time I'd have to resize it by hand otherwise I could not see the bottom.

What about if the Pharo window is just a quarter of a screen height and you then open a System Browser?

In that case you'd have to either resize the Pharo window so it takes more of the screen or the System Browser so it takes less. It would not be usable otherwise.

Using pixels as coordinates is definitely not the best measure Pharo could have taken. This is why this issue is occuring. For 4k resolution a similar problem would be that all the Pharo windows are too small. I am guessing all the Pharo devs are using screens with 1920 x 1080 resolutions. I wonder if there are problems over this with running Pharo on laptops?

[mrakgr]

Let me highlight again what I pointed in the original post. The way the openInWindow method is structured essentially makes no sense. You will see that in the first part it calls the RealEstateAgent which does translation and rescaling so that the window fits in the visible areas. Then in the second part, that window extent: essentially throws all of that away. I might not be familiar with Pharo, but my programmer's intuition is telling me that whoever wrote that part should have a second look. It is highly likely that the openInWindow method was intended to have rescalling behavior originally, but it got lost somewhere along the way.

[stale[bot]]

To limit bug bankruptcy (see https://www.joelonsoftware.com/2012/07/09/software-inventory/) this issue has been automatically marked as stale because it has not had any activity in 6 months. It will be closed in 1 month if no further activity occurs. If this issue remains important to you, please comment to reactivate the issue. Thank you for your contributions.

Joel on Software

Software Inventory

Imagine, for a moment, that you came upon a bread factory for the first time. At first it just looks like a jumble of incomprehensible machinery with a few people buzzing around. As your eyes adjus…