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

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

Open
mrakgr opened this Issue Mar 12, 2019 · 7 comments

Comments

Projects
None yet
2 participants
@mrakgr
Copy link

mrakgr commented Mar 12, 2019

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.

@mrakgr mrakgr changed the title Morph >> openInWindowLabeled:inWorld: violates the implicit usable area constraint Morph >> openInWindowLabeled:inWorld: can open windows outside the usable areas Mar 12, 2019

@mrakgr mrakgr changed the title Morph >> openInWindowLabeled:inWorld: can open windows outside the usable areas Morph >> openInWindowLabeled:inWorld: can open windows outside the usable area Mar 12, 2019

@bencoman

This comment has been minimized.

Copy link
Contributor

bencoman commented Mar 12, 2019

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

This comment has been minimized.

Copy link
Author

mrakgr commented Mar 12, 2019

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

This comment has been minimized.

Copy link
Contributor

bencoman commented Mar 12, 2019

@mrakgr

This comment has been minimized.

Copy link
Author

mrakgr commented Mar 12, 2019

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.

post-edit
pre-edit

mrakgr added a commit to mrakgr/Pharo-Examples that referenced this issue Mar 12, 2019

Let me stop here for the day.
pharo-project/pharo#2800

I opened this issue and got distracted with other things. I did not have time to play with Roassal, I'll save that for tomorrow.

Right now I've moved to the 8.0 version of Pharo. I guess I'll update every few months or something. I am not in a rush.

Let me leave that for later. I got 3 very long replies by /u/Antipurity on the Spiral's readme and tomorrow the first thing on the TODO list will be to reply to that. He really put in some effort so I'll do the same in turn. I flattered by such user participation. I never got questions from anybody.

After that comes Roassal finally. I'll put all of my effort into figuring out how to make those bar charts.

Let me call it a day here. It is time for food.
@bencoman

This comment has been minimized.

Copy link
Contributor

bencoman commented Mar 12, 2019

@mrakgr

This comment has been minimized.

Copy link
Author

mrakgr commented Mar 12, 2019

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

This comment has been minimized.

Copy link
Author

mrakgr commented Mar 12, 2019

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.

mrakgr added a commit to mrakgr/The-Spiral-Language that referenced this issue Mar 18, 2019

"9:35am.
```
type Actions =
    | Pass
    | Bet

type Cards =
    | One
    | Two
    | Three

let rng = System.Random()

let knuth_shuffle (ar: _[]) =
    let swap i j =
        let item = ar.[i]
        ar.[i] <- ar.[j]
        ar.[j] <- item

    for i=Array.length ar - 1 downto 1 do swap (rng.Next(i+1)) i

let ar = [|One; Two; Three|]
knuth_shuffle ar; ar
```

The Knuth shuffle is easy enough. I am using the version I saw in the Pharo library here.

Let me move on.

```
compare One Three
```

Actually, I am quite surprised that this gives me -2. I thought that compare only gave -1,0 and 1. Quite interesting.

Maybe it only does that for degenerate unions.

9:50am.

```
type State =
    | Pass
    | PassPass
    | PassBet
    | PassBetPass
    | PassBetBet
    | Bet
    | BetPass
    | BetBet
```

Let me put this here for the time being.

I haven't decided what the states should be yet.

10:15am.

```
let normalize array =
    let temp, normalizing_sum =
        Array.mapFold (fun s x ->
            let strategy = max x 0.0
            strategy, strategy + s
            ) 0.0 array

    let inline f g = for i=0 to temp.Length-1 do temp.[i] <- g temp.[i]
    if normalizing_sum > 0.0 then f (fun x -> x / normalizing_sum)
    else f (fun _ -> 1.0 / float actions.Length)

let add_strategy_sum agent realization_weight x =
    let sum = agent.strategy_sum
    Array.iteri (fun i x -> sum.[i] <- sum.[i] + realization_weight * x) x
```

Now `normalize` will be optimized.

10:25am. Renaming is just so good in VS. It is amazing.

In terms of stability and ease of use, VS is actually better than Pharo's IDE. Pharo needs to take some lessons from it.

Pharo irks:
- Lack of dedent with Shift + Tab
- Tabs rather than spaces
- [Undo](pharo-project/pharo#2814)
- Autocomplete sinking
- [System browser sinking](pharo-project/pharo#2800)
- GToolkit crashing on delete
- Lack of variable popup in debugger
- Meta taking only half of the screen
- [Class revert does not revert methods](pharo-project/pharo#2853)
- All the buggy Roassal examples

I am just gathering some ammo in case I get challenged in the next PL monthly thread.

Note: Highlight my recent renaming experience in VS.

10:35am. Now, enough of that. Let me start work on the CFR function before I get distracted any further.

As expected this example is quite hard, mostly because the code in the paper is so crappy.

I am going to have to do it roughly and then I am going to have to do it cleanly. I am not sure why there aren't two nodeMaps, one for each player.

10:45am.

```
private double cfr(int[] cards, String history, double p0, double p1) {
    int plays = history.length();
    int player = plays % 2;
    int opponent = 1 - player;
    *Return payoff for terminal states*
    String infoSet = cards[player] + history;
    *hGet information set node or create it if nonexistant*
    *For each action, recursively call cfr with additional history and probability*
    *For each action, compute and accumulate counterfactual regret*
    return nodeUtil;
}
```

No, this is just so crappy. I cannot possibly abide by this. I will do it differently.

Let me just ask, is this example really intended to have two players or is it just a single player playing against itself?

10:55am.

```
if (plays > 1) {
    boolean terminalPass = history.charAt(plays - 1) == ’p’;
    boolean doubleBet = history.substring(plays - 2, plays).equals("bb");
    boolean isPlayerCardHigher = cards[player] > cards[opponent];
    if (terminalPass)
        if (history.equals("pp")) return isPlayerCardHigher ? 1 : -1;
        else return 1;
    else if (doubleBet) return isPlayerCardHigher ? 2 : -2;
}
```

I will have to express this in terms of pattern matching.

11am.

```
let cfr history (one : Semblance) (two : Semblance) =
    match history with
    | [Pass; Pass] -> if one.card > two.card then 1.0 else -1.0
    | [Pass; Bet; Pass] -> -1.0
    | [Pass; Bet; Bet] -> if one.card > two.card then 2.0 else -2.0
    | [Bet; Pass] -> 1.0
    | [Bet; Bet] -> if one.card > two.card then 2.0 else -2.0
    | _ ->
```

Something like this should be decent.

11:15am.

```
double[] strategy = node.getStrategy(player == 0 ? p0 : p1);
double[] util = new double[NUM_ACTIONS];
double nodeUtil = 0;
for (int a = 0; a < NUM_ACTIONS; a++) {
    String nextHistory = history + (a == 0 ? "p" : "b");
    util[a] = player == 0
        ? - cfr(cards, nextHistory, p0 * strategy[a], p1)
        : - cfr(cards, nextHistory, p0, p1 * strategy[a]);
    nodeUtil += strategy[a] * util[a];
}
```

This is so convoluted. I have no idea which player is supposed to act here. I am going to have to do it as originally intended.

Let me take off for a bit here."
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.