#770 Place window on main screen if no secondary screen is present

[bluehazzard]

Reported here: http://forums.codeblocks.org/index.php/topic,22956.0.html

if the layout file places a window on the secondary screen, but you load a file on a pc without secondary screen the window is not visible...

[bluehazzard]

i tried to fix this with this code after loading the perspective file:

    wxAuiPaneInfoArray windowArray = m_LayoutManager.GetAllPanes();
    for (size_t i = 0; i < windowArray.GetCount(); ++i)
    {
        wxAuiPaneInfo pane = windowArray.Item(i);
        wxWindow* w = pane.window;
        int display = wxDisplay::GetFromWindow(w);
        if (display == wxNOT_FOUND)
        {
            // this window is not on a current display
            w->Move(0,0);
        }
    }

but the display seems to be always 0 if you use a extended desktop on windows...

btw. can someone document this function a bit:

void PlaceWindow(wxTopLevelWindow *w, cbPlaceDialogMode mode, bool enforce)

?

[Teodor Petrov]

Can you get the coordinates of the window?
What are they looking like? How does your client/desktop area values look like when you query the display object? (before and after you remove the display)
What is extended desktop?

[bluehazzard]

This patch fixes the problem. I am not sure if we should place it in the

void MainFrame::DoUpdateLayout()

function, but as far as i understand, this can only happen on loading perspectives...

[Teodor Petrov]

This looks good. But you need to do something about the size of the panes. You have to handle the case when the user has moved from 4k monitor to full hd or something even smaller.

I'd also be happier if the pane is moved in the center no at the left top corner.

Also you need to make formatting of the code consistent. Use space after if/for/etc. Use space after the comma in arguments to functions.
Use const more often.

[bluehazzard]

Ok, here is the second version of the patch.
This will also resize the window if it is to big for the target screen.
This probably will break someones work flow, but it fixes also this bug, what is more important...
Also added a lot comments so it all is a bit clearer

[Teodor Petrov]

1. dsp seems unused
2. please don't use abbreviations they are hard to read
3. why do you use place window? what is the benefit? Add a comment about it
4. pane.frame->GetSize() is called 4 times, curDsp.GetClientArea() is called 5 times. This is really bad pattern of laziness, please use variables for these! Or use variables for the width and height.
5. what is rat? no abbreaviations please
6. why is placewindow called twice? I doubt this is good idea.
7. also probably it is a good idea to move this in a separate named function.

[bluehazzard]

Ok, this are reasonable comments. Thank you!
Here is the new patch. I move the function to the global.cpp ,because this has nothing to do with the main class and can also be used ba plugins. For this i have incremented the SDK version

[Teodor Petrov]

Incrementing the version is needed only when you break the ABI. Adding a function doesn't break the ABI.

1. What is Nr? Number, Not requested, not ready... Idx or No are more common in this case.
2. Use cb prefix for the function name
3. Why do you duplicate the name of the function in the documentation comment?
4. "aspect ratio of the window allows it, it is saved." -> preserved is a better word than saved.
5. still I don't see uses of const, put it everywhere
6. 3.0 is double which makes the whole computation double. Use 3.0f if you don't want the extra precision
7. you're still copying windowArray and pane. You could probably use const& instead.

[bluehazzard]

Ok, i was thinking that the SDK version numbering (for codeblocks) is like incrementing the minor if you add function or make compatible bug fixes (do not break the API), increment major if you break API by changing function signatures and release... no idea why this is here.. If you do not increment one version number after adding a function, you can not target a plugin at this function... interesting reading about this: https://semver.org/

There is still some annoyance i can not fix if i do not break the API:
It is not a bug, only something annoying i hit during testing. I do not know if this hits people during work (it should only if the user loads the layout).
If a window on the second (valid) display is bigger then the second display, it will get resized, but placed at the center of the primary display (where codeblocks is located) and not where it was previously. This CAN be intentional behavior, but it is annoying... The size is calculated for the secondary display.
I could fix this with an additional parameter for "cbGetMonitorRectForWindow" and additional flags for "PlaceWindow". To allow the placement on the "current" display.

[Teodor Petrov]

This problems seems to happen for you because you want to reuse the PlaceWindow implementation. If you don't do it I guess you won't have this problem! You can also make an internal version of PlaceWindow, which is not part of the API/ABI.

[Teodor Petrov]

Also I don't think you should resize a window on the current display. Resize only if the window is on an invalid display!

[bluehazzard]

Also I don't think you should resize a window on the current display. Resize only if the window is on an invalid display!

this is not that easy... If the window ranges a bit in the active window it will be detected as beeing on a valid display. But you can not move the window if it is to big, or in the wrong place, so you have to resize it first. Then it will end on the wrong display and you have to move it to the right display: First resizing then detecting if it is on a valid display-> moving.

Also the problem with loading the layout from a other machine with bigger screen appears also on the main (valid) display, if the resolution are different. At least on windows the window will be to big on the first loading. I think this is the right approach at the moment. If someone complains i can think about a other solution.

[Teodor Petrov]

But you're resizing all windows? What happens if I want my window to be bigger than the screen? You'll resize it. This is something programs must not do!

You can get the rect of the screen and then ask wxDisplay if any of the corner points of the rect is on invalid display. Or even use some rect intersection to be even more robust.

But it is 100% guaranteed that you should do something only for windows that are bad. Regular windows must not be touched! You cannot predict what is the expected behaviour of any of the window managers on windows, linux or macos!

[bluehazzard]

I would like to touch the window only if the top border is not visible. As far as i can remember on all modern operating systems the window can only be moved if the border is visible (at least on windows, on linux you can probably move the window if you press the alt key) But i think we can generally assume that the top border should always be visible. If it is not visible we should relocate the window? Or am i wrong here?

[Teodor Petrov]

I'm not sure this is a good idea. I almost never move windows grabing the title bar. I always use a key and mouse combo. And I often move windows in a way that their title bar is not visible.

[bluehazzard]

I strongly prefer to ignore all the special cases.. If the window is not visible on the current display, rescale it and move it to the center of an active display... Finish there. No other checks or moves... If one little bit of the window is visible you can move it with the keyboard...

[bluehazzard]

Ok, this is my last patch. With this version the window is only relocated and resized if it is on an invalid display and to big to fit on this display.
I think this is the only version that won't interfere with the default behavior and fix the misbehavior

[edit:] wrong file...

[Teodor Petrov]

1. I still don't understand why you're calling place window...
2. displayNumber could be const
3. the alignment of = is already off ... one more reason why aligning code is a bad idea
4. making short ifs even shorter doesn't help readability
5. why are you using wxstring format without using a format string?
6. to help translators use _() instead of _T() or wxT()

Only 5 and 6 are show stoppers. Keep in mind that concatenated strings are impossible to translate, so a formatted string is better.

[Miguel Gimenez]

In addition to Teodor's comments:

• curentDisplay should be currentDisplay
• scaledWidth can be const
• The second parameter in the call to w->SetSize() should be scaledHeight

[bluehazzard]

Thank you for all the comments. I fixed them as far as i can tell.
I have tested this fix for wx30 and wx28 on windows. It works with wx30 but not with wx28. I do not know why, i tried to debug it and the code is called correctly but the window is not moved. I will not invest time in fixing this for wx28 because i think it won't affect so many user.
The hdd of my linux machine smoked up and i have not the possibility to replace it for the next few months so i can not test it on linux...

I will commit this patch as soon as my svn repo is ready and no one has something against it...

[bluehazzard]

Fixed in trunk r11553
Thank you all for the comments and the report