Bug 70642 – [QT] new API to get selection bounds for visibleSelection

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Srikumar B]

It seems like, there is no direct API available from QWebPage to get the text selection bounds co-ordinates.

The API logic may look similar to 

QRect QWebPage::getTextSelectionBounds()
{
    Frame* focusedFrame = d->page->focusController()->focusedFrame();
    if (!focusedFrame)
        return QRect();
    IntRect intRect=enclosingIntRect(focusedFrame->selection()->bounds());
    return QRect(intRect.x(),intRect.y(),intRect.width(),intRect.height());
}

[Srikumar B]

Kindly assign it to me, After confirming to add the new API.

[Srikumar B]

Created attachment 114206 [details]
patch for selectionBounds() API

proposed patch with the changes for adding new API selectionBounds()

[Srikumar B]

I wrote the API selctionBounds() to make use for more generic purpose for any application which used qtwebkit and can get visibleselection bounds.

This API sets the QRect reference variables for selection start position, selection end position & absolute bounding rect.

[Simon Hausmann]

Comment on attachment 114206 [details]
patch for selectionBounds() API

I'm going to say r-.

    1) The function is declared virtual even though it is not designed to be re-implemented.
    2) Adding virtual functions breaks binary compatibility (for no reason here)
    3) In Qt, out parameters are usually pointer types and the function signature starts with get.
    4) There is no unit test for this feature.

Can you elaborate what you're trying to do in your application? Why do you have to retrieve three rectangles?

I think we need to understand your use-case better. But in general I'm hesitant in adding new API to the WebKit "1" part of QtWebKit. We would like to consider it "done" and avod adding new features.

[Srikumar B]

Hi Simon,
Thanks for all your comments. I will upload a new patch with all the mentioned changes.

Regd the usecase, I am trying to draw selection markers at the start & end of selection for touch devices which does not have mouse pointer.
I believe this would be a better and generic approach to  get the selection details from FrameSelection.
please confirm your acceptance to add new API hence, i can upload the new patch with all your comments and Unit Test.

Thanks again


(In reply to comment #4)
> (From update of attachment 114206 [details])
> I'm going to say r-.
> 
>     1) The function is declared virtual even though it is not designed to be re-implemented.
>     2) Adding virtual functions breaks binary compatibility (for no reason here)
>     3) In Qt, out parameters are usually pointer types and the function signature starts with get.
>     4) There is no unit test for this feature.
> 
> Can you elaborate what you're trying to do in your application? Why do you have to retrieve three rectangles?
> 
> I think we need to understand your use-case better. But in general I'm hesitant in adding new API to the WebKit "1" part of QtWebKit. We would like to consider it "done" and avod adding new features.

[Srikumar B]

To share further details about the API signature, selectionStart & selectionEnd contain the dimensions of caret positions (x,y) with width=1 and height set to line height. boundingRect contain the QRect of complete selection. (x,y) position of selectionStart & boundingRect does not required to be same.

[Antonio Gomes]

Comment on attachment 114206 [details]
patch for selectionBounds() API

View in context: https://bugs.webkit.org/attachment.cgi?id=114206&action=review

It must come with auto tests as well.

> Source/WebKit/qt/Api/qwebpage.cpp:4175
> +/*!
> +    \fn bool QWebPage::selectionBounds(QRect& selectionStart, QRect& selectionEnd, QRect& boundingRect);
> +

Selection are per frame, so having it in WebPage, does not make sense to me.

Down the patch, you make use of FocusController::focusedOrMainFrame method, but a Frame can have text selection without being focused. Furthermore, two frames can have text selection simultaneously.

> Source/WebKit/qt/Api/qwebpage.cpp:4185
> +{
> +    selectionStart = selectionEnd = boundingRect = QRect();
> +    Frame* focusedFrame = d->page->focusController()->focusedFrame();

this is what I am talking about.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.