Autocomplete pop-ups appearing on wrong monitor in multi-headed systems

[SlySven]

As noted in the comment above TextEditorAutoCompleteComponent::positionWidgetForCaretOffset(int offset) which says:

This should be improved so border and screen positions are respected

there is a blooming great flaw in the code at least on X11 in a Linux environment with more than one monitor participating in a virtual desktop.

Whilst using Mudlet's version of edbee-lib (as revised by Dicene - possibly including some stuff from #75 that may not have been merged here) I found out that for my setup, comprising two 1680x1050 monitors in a vertical arrangement such that the upper monitor occupies (0,0) to (1679,1049) in Qt and X11 pixel coordinate terms and the lower (primary) one occupies (0,1050) to (1679,2099):

an auto-completion popup for a widget in the upper screen gets forced down to the lower screen, although doing the same when the widget is on the lower screen appears in the correct location:

The screen-shots do not show quite how bad this looks because the two separate screen areas are not so obviously separated in them compared to in real-life - the upper screen contains the application's main widow maximised so that the blue background represents that area that is on the other, lower, monitor.

:warning: Looking more closely at the code in that particular method I note that you are using Qt methods that have been declared obsolete, such as:

const QRect QDesktopWidget::availableGeometry(const QPoint &p) const

This function is obsolete. It is provided to keep old source code working. We strongly advise against using it in new code.

This is an overloaded function.

Returns the available geometry of the screen which contains p.

Use QGuiApplication::screenAt() instead.

See also screenGeometry().

I guess that the limits used to detect whether the menu needs to popup or popdown needs further revision to consider the correct limits when a virtual desktop is in use - because that will change how some of the screen geometry methods return values for the whole desktop rather than the specific screen on which a widget is drawn. I added some debugging code to report some of the details in each of the above two cases but I could not get my brain wrapped around how to improve the code to use the right limits and then apply the right correction - but I believe the method I mentioned at the top of this issue is the guilty culprit...! :thinking:

[gamecreature]

On my Mac it simply works. I will try to change it, so it uses screenAt

[gamecreature]

This is pretty strange.. The TextEditorAutoCompleteComponent uses the TextEditorComponent as Parent Component. This means the move coordinate is relative to the coordinate system of the parent

Do you know what methods I use are conflicting or deprecated? (I don't use availableGeometry)

[sebcaux]

QWidget::mapToGlobal

[gamecreature]

In which source file is this used? I cannot find it with grep in the sources of edbee-lib. It uses a move, which moves relative to the parent's widget:

https://github.com/edbee/edbee-lib/blob/master/edbee-lib/edbee/views/components/texteditorautocompletecomponent.cpp#L124

[SlySven]

Well you are using QDesktopWidget::availableGeometry(...) and both forms that take an argument (a pointer to a widget OR a screen number) were declared obsolete I seen in Qt 5.11.0 ...

In the situation I think I was seeing that call (in two places) returning something like QRect(QPoint(0,0), QPoint(1679,2099)) which should not have run foul of the clipping code in (void) TextEditorAutoCompleteComponent::positionWidgetForCaretOffset(int).

Actually running with some debug code in place:

void TextEditorAutoCompleteComponent::positionWidgetForCaretOffset(int offset)
{
    qDebug() << "TextEditorAutoCompleteComponent::positionWidgetForCaretOffset(" << offset << ") :";
    // find the caret position
    TextRenderer* renderer = controller()->textRenderer();
    qDebug() << "    Viewport (X,Y)" << controller()->textRenderer()->viewportX() << "," << controller()->textRenderer()->viewportY();
    int y = renderer->yPosForOffset(offset) - controller()->textRenderer()->viewportY(); //offset the y position based on how far scrolled down the editor is
    int x = renderer->xPosForOffset(offset) - controller()->textRenderer()->viewportX() + marginComponentRef_->widthHint() - 3; //adjusts x position based on scrollbars, line-number, and position in line
    qDebug() << "    X:" << x << " Y:" << y;
    QPoint newLoc = editorComponentRef_->parentWidget()->parentWidget()->mapToGlobal(QPoint(x, y));

    //We want to constrain the list to only show within the available screen space
    QRect screen = QApplication::desktop()->availableGeometry(this);
    qDebug().nospace().noquote() << "   screen.left: " << screen.left() << " screen.top: " << screen.top() << " screen.width: " << screen.width() << " screen.height: " << screen.height() << " screen.bottom: " << screen.bottom();
    qDebug() << "   Initial newLoc (X,Y)" << newLoc.x() << "," << newLoc.y() << ")";
    newLoc.setX(qMax(screen.left(), newLoc.x()));                                   //constrain the origin of the list to the leftmost pixel
    newLoc.setY(qMax(screen.top(), newLoc.y()));                                    //constrain the origin of the list to the topmost pixel
    newLoc.setX(qMin(screen.x() + screen.width() - menuRef_->width(), newLoc.x())); //ensure that the entire width of the list can be shown to the right
    if( newLoc.y() + menuRef_->height() > screen.bottom() ){                        //if the list could go below the bottom, draw above
        newLoc.setY(qMin(newLoc.y(), screen.bottom()) - menuRef_->height());        //positions the list above the word
    } else {
        newLoc.setY(newLoc.y() + renderer->lineHeight()); //places it below the line, as normal
    }
    qDebug() << "   Adjusted newLoc (X,Y)" << newLoc.x() << "," << newLoc.y() << ")";
    menuRef_->move(newLoc.x(), newLoc.y());
}

With the parent window in the lower screen (and the popup in the correct place) I got:

TextEditorAutoCompleteComponent::positionWidgetForCaretOffset( 129 ) :
    Viewport (X,Y) 0 , 0
    X: 37  Y: 76
   screen.left: 0 screen.top: 1050 screen.width: 1680 screen.height: 1050 screen.bottom: 2099
   Initial newLoc (X,Y) 591 , 1392 )
   Adjusted newLoc (X,Y) 591 , 1411 )

However with the parent window in the upper screen I got:

TextEditorAutoCompleteComponent::positionWidgetForCaretOffset( 129 ) :
    Viewport (X,Y) 0 , 0
    X: 37  Y: 76
   screen.left: 0 screen.top: 1050 screen.width: 1680 screen.height: 1050 screen.bottom: 2099
   Initial newLoc (X,Y) 561 , 377 )
   Adjusted newLoc (X,Y) 561 , 1069 )

To me the value of screen.top and screen.bottom do not seem right as they are not changing when the window containing the edbee-lib widget is being moved between the two monitors comprising my Desktop...

[SlySven]

Oh, humblest, grovelling apologies - it is @dicene 's fork (https://github.com/dicene/edbee-lib) that has improved your code to the extent that it is broken :frowning_face: ...

[gamecreature]

Haha no problem! I guess that positioning the widget should be kept relative top the parent window. I don't think it's required to know which screen it's on.. Simply check if the popup should be above or below the cursor depending on the location in the editor.

[SlySven]

And a fix (at least for Qt 5.10.0 and later) is proposed at https://github.com/dicene/edbee-lib/pull/1 .

Simply check if the popup should be above or below the cursor depending on the location in the editor.

... it seems that should be above or below the cursor is dependent on whereabouts on the screen the cursor is and that needs to know which screen it is being shown on to get the range of usable coordinates!