Bug 44334 – [EFL] Text selection advanced API

• Component: WebKit EFL
• Status: RESOLVED WONTFIX

[Mirek Szymanski]

This patch contains new methods in EFL port to control text selection:
 - select a word under given position
 - return positions of upper left/bottom right corner of the selection
 - modify left/right position of the selection
 - check if there is selection at given position

Regards,
Miroslaw

[Mirek Szymanski]

Created attachment 64954 [details]
Implement API to manage text selection

[Rafael Antognolli]

Hi Mirek,

The patch is nice, but could you please consider adding some doxygen comments on each of these functions, describing the parameters and return values?

I also couldn't get the reason why you need top_h and bottom_h parameters, but I think the documentation will be enough for me to understand it.

Another change if you want to make the API more versatile is here:

+Eina_Bool ewk_frame_selection_handlers_get(
+        Evas_Object* o,
+        int* top_x, int* top_y, int* top_h, 
+        int* bottom_x, int* bottom_y, int* bottom_h)
+{
+    EWK_FRAME_SD_GET_OR_RETURN(o, sd, EINA_FALSE);
+    EINA_SAFETY_ON_NULL_RETURN_VAL(sd->frame, EINA_FALSE);
+    EINA_SAFETY_ON_NULL_RETURN_VAL(top_x, EINA_FALSE);
+    EINA_SAFETY_ON_NULL_RETURN_VAL(top_y, EINA_FALSE);
+    EINA_SAFETY_ON_NULL_RETURN_VAL(top_h, EINA_FALSE);
+    EINA_SAFETY_ON_NULL_RETURN_VAL(bottom_x, EINA_FALSE);
+    EINA_SAFETY_ON_NULL_RETURN_VAL(bottom_y, EINA_FALSE);
+    EINA_SAFETY_ON_NULL_RETURN_VAL(bottom_h, EINA_FALSE);

Instead of failing if any of the parameters is NULL, you can check each of them before assigning its value, like the function ewk_view_viewport_get(). This would allow the user to fetch just some of the values, without the need to create dummy variables to receive the other parameters.

This comment is valid for all these new functions you've created.

Regards,
Rafael

[Mirek Szymanski]

Thanks Rafael for the comments.  I'll get back to this patch later, when I'm back from vacations.

-Mirek

[Adam Barth]

Comment on attachment 64954 [details]
Implement API to manage text selection

Clearing the review flag since miroslaw.s will get back to this patch later.

[Lucas De Marchi]

Hi, Mirek

(In reply to comment #3)
> Thanks Rafael for the comments.  I'll get back to this patch later, when I'm back from vacations.
> 

Are you planning to re-submit this?

[Ryuan Choi]

Created attachment 84688 [details]
Patch

[Ryuan Choi]

(In reply to comment #6)
> Created an attachment (id=84688) [details]
> Patch

Instead of mirek, I upload patch including rafael's comment.

Especially, I removed top_h and bottom_h parameters because it's special case we are using.

[Ryosuke Niwa]

Comment on attachment 84688 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_frame.cpp:761
> +    selectedRange->textRects(rects, true);

selectedRange can be null

> Source/WebKit/efl/ewk/ewk_frame.cpp:785
> +Eina_Bool ewk_frame_select_closest_word(Evas_Object* o, int x, int y, Eina_Rectangle* left_handle, Eina_Rectangle* right_handle)

This function seems to duplicate what EventHandler::selectClosestWordFromMouseEvent implements.  Can we share code with that function somehow?

[Ryosuke Niwa]

Comment on attachment 84688 [details]
Patch

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

>> Source/WebKit/efl/ewk/ewk_frame.cpp:761
>> +    selectedRange->textRects(rects, true);
> 
> selectedRange can be null

r- because of this.

[Ryuan Choi]

(In reply to comment #9)
> (From update of attachment 84688 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84688&action=review
> 
> >> Source/WebKit/efl/ewk/ewk_frame.cpp:761
> >> +    selectedRange->textRects(rects, true);
> > 
> > selectedRange can be null
> 
> r- because of this.

Thank you for your review.
I'll fix and check EventHandler::selectClosestWordFromMouseEvent.

[Ryuan Choi]

Created attachment 95599 [details]
Patch

[Ryuan Choi]

(In reply to comment #9)
> (From update of attachment 84688 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84688&action=review
> 
> >> Source/WebKit/efl/ewk/ewk_frame.cpp:761
> >> +    selectedRange->textRects(rects, true);
> > 
> > selectedRange can be null
Done.

> 
> r- because of this.

Despite of your kindness,
I can't find easy way to use EventHandler::selectClosestWordFromMouseEvent on WebKit/EFL without events.

[Rafael Antognolli]

(In reply to comment #12)
> (In reply to comment #9)
> > (From update of attachment 84688 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=84688&action=review
> > 
> > >> Source/WebKit/efl/ewk/ewk_frame.cpp:761
> > >> +    selectedRange->textRects(rects, true);
> > > 
> > > selectedRange can be null
> Done.
> 
> > 
> > r- because of this.
> 
> Despite of your kindness,
> I can't find easy way to use EventHandler::selectClosestWordFromMouseEvent on WebKit/EFL without events.

Hello Ryuan,

I think Ryosuke isn't talking about using EventHandler::selectClosestWordFromMouseEvent directly, but in someway refactor that function so it would call another function. This other function would have the common code available to be called by ewk_frame_select_closest_word.

[Gyuyoung Kim]

Ryuan,

This bug is too old. Any news on this bug ?

[Kenneth Rohde Christiansen]

Comment on attachment 95599 [details]
Patch

I don't see why you ned to export such an API. In Qt (not all upstreamed) we don't do that and we have quite good selection/composition support for our virtual keyboard.

[Ryuan Choi]

(In reply to comment #15)
> (From update of attachment 95599 [details])
> I don't see why you ned to export such an API. In Qt (not all upstreamed) we don't do that and we have quite good selection/composition support for our virtual keyboard.

OK, I'll check and try to find proper way.

I'll close this until I find better way.