Set the kps window position to the center of the screen

[efojug]

Set the kps window position to the center of the screen #94

[RoanH]

Hello, thank you for the contribution!

That was the call I was planning to add yeah, though as it is right now this would actually override the window position the user has configured via their configuration file. I think it's probably fine to only set the window position if a position isn't set via the config file though. However, it probably wouldn't be a bad idea to double check that considering how some of the config loading logic is still a bit of a mess. A bit more tricky to solve though is that placing the call here places the top left corner of the window in the center of the screen instead of the center of the window. Probably changing the visibility and location after the window was reconfigured should work for that, but I'm not entirely sure that would always work.

[efojug]

I've updated the code so that the window is created so that the center of the window is centered to the screen, which shouldn't affect the application of the existing configuration file

[RoanH]

Hey, this strategy looks like it should work fine.

Some comments:

1. This will still override the user configured window position, so it shouldn't run if PositionSettings#hasPosition is true.
2. We can just use setLocationRelativeTo(null) like in your initial version, the only reason that did not work before was because there was nothing in the frame (since reconfigure adds the content). That method also properly handles the case where (0, 0) is not the top left of the display (as is the root cause of #94).
3. The frame already has a window listener (CloseListener), so we can probably just add the logic there (and probably rename the class to MainWindowListener or something).

[efojug]

It should be available now. :3

[RoanH]

Hi, thanks for the quick update!

Sorry, but here are some more comments 😅

1. When the buildGUI function runs the user passed position is already set, so there's no need to set it again.
2. You can't add the x and y coordinate of the position to determine if it's unset, if the user configured position is actually (0,0) that would move it to the center, but also note that negative coordinates are possible. Positions left of the main monitor are negative so a position of say (-100, 100) is possible. That's why I added a hasPosition method to check if a position is present or not.
3. Not sure why you removed the window listener? Without the window listener it now centers the top left corner again instead of the entire window, since the frame has a size of 0x0 during the buildGUI function. If you changed that because of the 3rd point from my previous message, that was referring to not adding a second window listener and instead reusing the currently existing CloseListener (and possibly renaming it).

[efojug]

Sorry, I misunderstood your previous reply. It should be working now.

[RoanH]

Yep, that was the approach I was thinking of.

I made some minor changes and fixed a bug I introduced yesterday that caused the position to be set incorrectly... Note that the regular config handling already sets the position if one is present, so the window handler only needed to handle the case where the is no position. Also note that setLocationRelativeTo is a more robust method so it's preferable over computing the location manually, and if you do compute it manually you need to add in the coordinates of the window because the primary display is not always at (0,0).

Anyway, thanks again for the contribution!

[RoanH]

I'll merge this if you're fine with the changes I made.

[efojug]

Okay, thanks for the guidance.

[efojug]

Hmm. your tweaked code doesn't seem to set the default configuration window position correctly

[RoanH]

The default position is set by the ConfigLoader class (moved to Main#applyConfig now), which is called from either the main dialog or the right click menu (the latter being why a window listener cannot set it correctly). There is a separate bug when it comes to loading CLI/default configurations though, which is the root cause behind #92, but I'll fix that one in a bit.

[RoanH]

I'm going to merge this to prevent another merge conflict, but let me know if you can reproduce a case where a default position isn't applied correctly.