Bug 54439 – [EFL] New API for executing Editor Commands: InsertText, InsertImage.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kamil Blank]

Created attachment 82425 [details]
patch

I'd like to add new functionalities to be executed by EditorCommand - InsertText, InsertImage.
I decided to do it in a different way than previous commands: "SelectWord", "SelectLine" etc. 
I believe that having one function for all commands is much better than tens of similar functions 
(what is possible as EditorCommand has a lot of commands defined).

Due to this fact I was also thinking about replacing existing functions (listed below) by additional enums in Ewk_Editor_Command
but I'm not sure whether removing API is a good idea, what do you think?

List of existing editor commands:
EAPI Eina_Bool    ewk_view_select_none(Evas_Object *o);
EAPI Eina_Bool    ewk_view_select_all(Evas_Object *o);
EAPI Eina_Bool    ewk_view_select_paragraph(Evas_Object *o);
EAPI Eina_Bool    ewk_view_select_sentence(Evas_Object *o);
EAPI Eina_Bool    ewk_view_select_line(Evas_Object *o);
EAPI Eina_Bool    ewk_view_select_word(Evas_Object *o);

[WebKit Review Bot]

Attachment 82425 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/efl/ewk/ewk_view.h:363:  The parameter name "o" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/efl/ewk/ewk_view.h:363:  The parameter name "command" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

(In reply to comment #1)
> Attachment 82425 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1
> 
> Source/WebKit/efl/ewk/ewk_view.h:363:  The parameter name "o" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Source/WebKit/efl/ewk/ewk_view.h:363:  The parameter name "command" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Total errors found: 2 in 3 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

To fix these style errors, you have to remove parameter name as below,

EAPI Eina_Bool    ewk_view_execute_editor_command(Evas_Object*, const Ewk_Editor_Command, const char *value);

However, other APIs have parameter name in ewk_view.h. I don't know if we should remove parameter name in this patch. 

Lucas, how do you think about this ?

[Gyuyoung Kim]

Comment on attachment 82425 [details]
patch

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

> Source/WebKit/efl/ChangeLog:6
> +

Please add this bug url to here

[Kamil Blank]

Created attachment 82433 [details]
patch

+ url added

[WebKit Review Bot]

Attachment 82433 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/efl/ewk/ewk_view.h:363:  The parameter name "o" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/efl/ewk/ewk_view.h:363:  The parameter name "command" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kamil Blank]

> To fix these style errors, you have to remove parameter name as below,
It's probably about the names which are used here.
Looks like check-webkit-style doesn't accept names like: "o" and "command".
As ewk uses "o" for "object" - I don't think it's a good idea to change it in one place.
I can only change "command" but there won't be better name for it.

[Gyuyoung Kim]

I add WebKit EFL reviewers to CC

[Antonio Gomes]

Comment on attachment 82433 [details]
patch

Waiting for EFL API review...

[Kamil Blank]

Created attachment 83059 [details]
patch

Patch adjusted to the style fixes in ewk_view.h

[Kamil Blank]

Created attachment 83061 [details]
patch

Removed unnecessary line

[Lucas De Marchi]

Comment on attachment 83061 [details]
patch

LGTM

[Gyuyoung Kim]

Comment on attachment 83061 [details]
patch

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

> Source/WebKit/efl/ewk/ewk_view.h:365
> +EAPI Eina_Bool    ewk_view_execute_editor_command(Evas_Object*, const Ewk_Editor_Command, const char *value);

Please add parameters to this API. Style error checker doesn't recognize ewk header files now.

[Kamil Blank]

Created attachment 84587 [details]
patch

Restored params names.

[Kamil Blank]

Could someone please review this patch?

[Lucas De Marchi]

(In reply to comment #0)
> Created an attachment (id=82425) [details]
> patch
> 
> I'd like to add new functionalities to be executed by EditorCommand - InsertText, InsertImage.
> I decided to do it in a different way than previous commands: "SelectWord", "SelectLine" etc. 
> I believe that having one function for all commands is much better than tens of similar functions 
> (what is possible as EditorCommand has a lot of commands defined).
> 
> Due to this fact I was also thinking about replacing existing functions (listed below) by additional enums in Ewk_Editor_Command
> but I'm not sure whether removing API is a good idea, what do you think?
> 
> List of existing editor commands:
> EAPI Eina_Bool    ewk_view_select_none(Evas_Object *o);
> EAPI Eina_Bool    ewk_view_select_all(Evas_Object *o);
> EAPI Eina_Bool    ewk_view_select_paragraph(Evas_Object *o);
> EAPI Eina_Bool    ewk_view_select_sentence(Evas_Object *o);
> EAPI Eina_Bool    ewk_view_select_line(Evas_Object *o);
> EAPI Eina_Bool    ewk_view_select_word(Evas_Object *o);


IMO it's ok to remove these too, and merge with this new one that you implemented. Feel free to add this later.

thanks

[Antonio Gomes]

Comment on attachment 84587 [details]
patch

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

> Source/WebKit/efl/ewk/ewk_view.h:361
> +    EWK_EDITOR_COMMAND_INSERT_TEXT = 1

you do not need "= 1" here.

[Lucas De Marchi]

Hi, Kamil


Please, remove the "= 1" Antonio mentioned before landing this patch. We do our best to enforce webkit's coding style.


Thanks.

[Kamil Blank]

Created attachment 91263 [details]
patch

Removed unnecessary code from previous patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 91263 [details]:

java/lc3/JSObject/ToObject-001.html bug 53091 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 91263 [details]
patch

Clearing flags on attachment: 91263

Committed r85295: <http://trac.webkit.org/changeset/85295>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/85295 might have broken Windows 7 Release (Tests)