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

Odd behaviour when opening a patch made on an earlier version (on Windows) #1805

Closed
bazilmuzik opened this issue Oct 27, 2022 · 28 comments · Fixed by #1804
Closed

Odd behaviour when opening a patch made on an earlier version (on Windows) #1805

bazilmuzik opened this issue Oct 27, 2022 · 28 comments · Fixed by #1804
Labels
os:windows issues primarily affecting Windows pending this issue is resolved, but not released/merged into master yet regression for problems that did not exist in previous releases subject:GUI things concerning the GUI side

Comments

@bazilmuzik
Copy link

So I just installed version 0-53 and I noticed that when I open patches I recently made (99.9% of them), the window doesn't show up.
Instead, I have to hover my mouse on the Pd icon in the toolbar (bottom of my screen), right click on the patch window and then choose the option "maximize" in order to see my patch. See attached image (the patch I opened and which doesn't show up right away is named sequence_wt.pd)
And even after that, I have to do the exact same thing when I try to open a subpatch.

Windows_issue

@Lucarda
Copy link
Contributor

Lucarda commented Oct 28, 2022

Hi,

Strange. I can't reproduce on Win11 with Pd-0.53.

Does the console print something?

Can you share a patch that exhibits the problem? (compress it to ZIP and upload in the comment)

EDIT:

Is there a chance that those patches were saved in a secondary display and now that display is plugged but turned off?

@ben-wes
Copy link
Contributor

ben-wes commented Oct 28, 2022

I had the same issue yesterday. I'll try to analyize it more precisely later - but it seems to be related to setups with multiple screens. The patches I built with 2 screens all seemed to open in a hidden place (as for @bazilmuzik), although I edited them on the primary screen in order to not cause weird behavior for people with a different setup when they open them. And even after moving and resizing the windows, the behavior was still weird.

I was now briefly testing with just one screen on my laptop on Win10, Pd-0.53 (patches that were build with the prior Pd versions) and can't reproduce it here neither.

@umlaeute
Copy link
Contributor

Weird. I don't recall any changes to the windowing code...

@umlaeute umlaeute added os:windows issues primarily affecting Windows regression for problems that did not exist in previous releases subject:GUI things concerning the GUI side labels Oct 28, 2022
@ben-wes
Copy link
Contributor

ben-wes commented Oct 28, 2022

Direct follow-up since I was curious now and connected a second screen ... I think I was wrong with my assumption that it's related to patches that were edited on the primary screen. This behaves correctly here - at least for new patches (will have to check older ones later). But if I save a patch with its patch window on the second screen and close/reopen it, I get the weird behavior we're reporting here. This was not the case before.

Here's the resulting file for a test patch I saved on the second screen:

#N canvas 681 -770 450 300 12;
#X text 49 49 test on second screen above primary screen;

@Lucarda
Copy link
Contributor

Lucarda commented Oct 28, 2022

This was the last change I can recall for Windows: #1619

@ben-wes
Copy link
Contributor

ben-wes commented Oct 28, 2022

I just reinstalled Pd-0.52-2 to compare. If I open the little sample patch above, it's getting displayed correctly at the position where I edited it. Seems like it's an issue of correctly displaying windows with negative coordinates maybe?

@ben-wes
Copy link
Contributor

ben-wes commented Oct 28, 2022

Tested a bit more on Pd-0.53 with 2 vertically aligned screens. This file here appears again partly on the primary screen at the bottom:

#N canvas 0 -1200 453 301 12;

@ben-wes
Copy link
Contributor

ben-wes commented Oct 28, 2022

... so if I understand the behavior of Pd correctly here, it looks like this to me:

  • for Pd-0.53, if window coordinates are outside the available display size (including both screens), the position is wrapped for the available width and height. This causes the issue because it gets obviously wrapped around the total height, which is 2160px for me, but my display y coordinates range from -1080 to 1080. So the window gets positioned at an invisible area for patches with a y-position between -1 and -1080 (resulting in an actual positioning between 2159 and 1080, I guess).
  • Pd-0.52-2 doesn't seem to have this "wrapping" mechanism. Patches that were saved at some coordinate that's not visible when reopening them, are just not visible.

So I really like the approach of the new version with the wrapping! It just doesn't seem to work for setups where the upper left coordinate of the available total display is negative. But it actually looks like a bug in a new feature and not a regression to me.

@ben-wes
Copy link
Contributor

ben-wes commented Oct 28, 2022

Could this be related to the changes in this context maybe? 10191a6
Also wanted to add: if I use my upper screen as primary screen, everything works fine - with a range from (0, 0) to (1920, 2160). But ovbiously (and unfortunately), these negative coordinates are not uncommon (at least for Windows).

EDIT: more precisely, it's probably these lines not expecting negative actual screen coordinates?

set x [ expr $x % $width]
set y [ expr $y % $height]

@Lucarda
Copy link
Contributor

Lucarda commented Oct 28, 2022

@ben-wes thnks for your findigs.

Can you do some test?

backup (make a copy of) pd-0.53-0/tcl/pdtk_canvas.tcl to pd-0.53-0/tcl/pdtk_canvas.tcl.bak or something you like.

then edit pd-0.53-0/tcl/pdtk_canvas.tcl line 66-69 to:

        set x [ expr $x % $width]
        set y [ expr $y % $height]
        if {$x < [lindex [wm minsize .] 0]} {set x 0}
        if {$y < [lindex [wm minsize .] 1]} {set y 0}

save the file and restart Pd. Then please do all kinds of tests. I think this fix will handle all situations.

@ben-wes
Copy link
Contributor

ben-wes commented Oct 28, 2022

@Lucarda Thanks for the snippet! Unfortunately, it didn't solve the problem. I tried to find some documentation on this Tcl stuff concerning the case with negative screen coordinates to test around a bit myself ... that was a bit of a pain - but in the end, these lines here seem to work well for all my tested configurations (handling a negative offset correctly for negative screen origin coordinates):

        set xmin [winfo vrootx .]
        set ymin [winfo vrooty .]
        set x [expr ($x - $xmin) % $width + $xmin] 
        set y [expr ($y - $ymin) % $height + $ymin] 

@Lucarda
Copy link
Contributor

Lucarda commented Oct 28, 2022

@ben-wes fantastic, I was just reading about winfo vrootx/y

I'm going to test your code but surely it works :)

@Lucarda
Copy link
Contributor

Lucarda commented Oct 28, 2022

@ben-wes this looks totally good for me. (only tested it with a single monitor)

I think you already test with dual monitor.

Do you feel ok to do a PR? (you know how to use git and all that?)

In case you don't want to do the PR just tell me and I'll do it.

@ben-wes
Copy link
Contributor

ben-wes commented Oct 28, 2022

Good to hear, @Lucarda ! ... I tested with a single monitor and two in different layouts. Can do some additional testing with irregular layouts though! And are the names xmin and ymin alright for the screen origin? I haven't really checked other conventions there.

And I'll happily do a PR - although I never did that before, I admit. But maybe it's good to start at some point. :)

Will do both (more tests and PR) later today or tomorrow though since I'm currently not at home, ok?

@Lucarda
Copy link
Contributor

Lucarda commented Oct 28, 2022

@ben-wes sure no trouble and no rushing.

I'll try to do more tests in the mean time. Also I might be away from my computers in the weekend.

Feel free to ask here whatever. You might also read this https://github.com/pure-data/pure-data/wiki/Pull-Requests . Your PR fit to be aimed at develop branch.

@Lucarda
Copy link
Contributor

Lucarda commented Oct 28, 2022

oops I did some multi-monitor tests and failed. We will have to dig more

@Lucarda
Copy link
Contributor

Lucarda commented Oct 28, 2022

false alarm to my post above :)

I was testing with an incorrect pdtk_canvas.tcl. The good news is that I had tested with with multi-monitor connected and it seems good with all settings.

@ben-wes
Copy link
Contributor

ben-wes commented Oct 28, 2022

haha, ok... maybe it's not gonna be the first PR for me. ;) ... do you have any details on how you were testing? resolution of the screens and how they were aligned?

@ben-wes
Copy link
Contributor

ben-wes commented Oct 28, 2022

ah! ok... thanks! anyway - I'll do some tests with untypical monitor configurations.

@Lucarda
Copy link
Contributor

Lucarda commented Oct 28, 2022

monitors

positioning the 2nd monitor at north, east, south or west from 1st monitor.

@ben-wes
Copy link
Contributor

ben-wes commented Oct 28, 2022

Tested a bit more now. Of course, it's still possible to hide the patch window with a setup like this:

image

... but I assume that's expected?

Besides that, I found the following things which might be acceptable - but still wanted to mention these before proceeding here:

  • x coordinate in my 1920x1080 screen setup only wraps at 1924px (EDIT: tested for layout with secondary screen on the left of primary screen)
  • y coordinate already wraps at 1041px (EDIT: tested for layout with secondary screen on the left of primary screen)
  • it's becoming a mess when screens have different system zoom settings (although this seems logical, but pixels are not handled as actual pixels anymore then obviously)
  • fullscreen mode is not getting saved. When reloaded, this gets set back to the last positioned/scaled window properties (it's the same for the current version and acceptable imho - but not exactly what I would expect)

If there's no need to work on these other points, I'll do the PR tomorrow.

@Lucarda
Copy link
Contributor

Lucarda commented Oct 28, 2022

@ben-wes thanks for working on this.

If there's no need to work on these other points, I'll do the PR tomorrow.

I'm not sure if any of us knows how to deal with those points. If you ask me: go ahead with the PR: we surely need that change in the code.

PS: remember to create your new branch on top of develop

git pull --all

git checkout develop

git checkout -b <you-name-it>

...

later you can target develop instead of master when you create the PR via the webrowser from your fork.

image

pr

@ben-wes
Copy link
Contributor

ben-wes commented Oct 29, 2022

Thanks for your help and documentation here, @Lucarda ! I just created the PR. I'm not quite sure if I did it all correctly concerning how issue and PR are linked now. Possibly not. Hope, this can be handled correctly anyway.

@Lucarda
Copy link
Contributor

Lucarda commented Oct 29, 2022

@ben-wes the PR looks perfectly good. Well done!.

I'll only suggest that you add more info in the description like:


This PR is to be merged into develop branch`

Closes: #1805

....


@ben-wes
Copy link
Contributor

ben-wes commented Oct 29, 2022

Thanks for your feedback @Lucarda ! I updated the description accordingly.

@Lucarda
Copy link
Contributor

Lucarda commented Oct 29, 2022

👍

@umlaeute umlaeute added the pending this issue is resolved, but not released/merged into master yet label Oct 31, 2022
@bazilmuzik
Copy link
Author

Thanks to all of you for looking at that, I wouldn't have been able to dig that deep into the issue :)

@ben-wes
Copy link
Contributor

ben-wes commented Oct 31, 2022

Thanks for opening the issue, @bazilmuzik ! I was glad to see your report here when I was actually about to create one myself - because then I realized I was not the only one experiencing this weird behavior and it motivated me to look deeper into it since it was obviously Pd and not my system (which I wasn't sure about in the beginning).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os:windows issues primarily affecting Windows pending this issue is resolved, but not released/merged into master yet regression for problems that did not exist in previous releases subject:GUI things concerning the GUI side
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants