Bug 54439

Summary: [EFL] New API for executing Editor Commands: InsertText, InsertImage.
Product: WebKit Reporter: Kamil Blank <k.blank>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, antognolli+webkit, commit-queue, eric, gyuyoung.kim, kenneth, leandro, lucas.de.marchi, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 54624    
Bug Blocks:    
Attachments:
Description Flags
patch
none
patch
none
patch
none
patch
gyuyoung.kim: commit-queue-
patch
tonikitoo: review+, lucas.de.marchi: commit-queue-
patch none

Kamil Blank
Reported 2011-02-15 00:09:29 PST
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);
Attachments
patch (3.15 KB, patch)
2011-02-15 00:09 PST, Kamil Blank 		no flags
DetailsFormatted DiffDiff
patch (3.20 KB, patch)
2011-02-15 02:50 PST, Kamil Blank 		no flags
DetailsFormatted DiffDiff
patch (3.13 KB, patch)
2011-02-18 23:47 PST, Kamil Blank 		no flags
DetailsFormatted DiffDiff
patch (3.13 KB, patch)
2011-02-19 00:01 PST, Kamil Blank 		gyuyoung.kim: commit-queue-
DetailsFormatted DiffDiff
patch (3.19 KB, patch)
2011-03-03 10:58 PST, Kamil Blank 		tonikitoo: review+
lucas.de.marchi: commit-queue-
DetailsFormatted DiffDiff
patch (3.15 KB, patch)
2011-04-27 03:50 PDT, Kamil Blank 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
WebKit Review Bot
Comment 1 2011-02-15 00:12:25 PST
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
Comment 2 2011-02-15 02:16:29 PST
(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 3 2011-02-15 02:22:29 PST
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
Comment 4 2011-02-15 02:50:02 PST
Created attachment 82433 [details] patch + url added
WebKit Review Bot
Comment 5 2011-02-15 02:52:31 PST
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
Comment 6 2011-02-15 03:38:31 PST
> 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
Comment 7 2011-02-15 22:38:57 PST
I add WebKit EFL reviewers to CC
Antonio Gomes
Comment 8 2011-02-17 13:37:41 PST
Comment on attachment 82433 [details] patch Waiting for EFL API review...
Kamil Blank
Comment 9 2011-02-18 23:47:07 PST
Created attachment 83059 [details] patch Patch adjusted to the style fixes in ewk_view.h
Kamil Blank
Comment 10 2011-02-19 00:01:05 PST
Created attachment 83061 [details] patch Removed unnecessary line
Lucas De Marchi
Comment 11 2011-02-21 04:43:12 PST
Comment on attachment 83061 [details] patch LGTM
Gyuyoung Kim
Comment 12 2011-03-02 00:00:11 PST
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
Comment 13 2011-03-03 10:58:05 PST
Created attachment 84587 [details] patch Restored params names.
Kamil Blank
Comment 14 2011-04-21 06:11:14 PDT
Could someone please review this patch?
Lucas De Marchi
Comment 15 2011-04-26 16:42:17 PDT
(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 16 2011-04-26 16:46:15 PDT
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
Comment 17 2011-04-27 02:33:22 PDT
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
Comment 18 2011-04-27 03:50:51 PDT
Created attachment 91263 [details] patch Removed unnecessary code from previous patch.
WebKit Commit Bot
Comment 19 2011-04-28 20:32:42 PDT
The commit-queue encountered the following flaky tests while processing attachment 91263 [details]: java/lc3/JSObject/ToObject-001.html bug 53091 (author: ap@webkit.org) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 20 2011-04-28 20:34:02 PDT
Comment on attachment 91263 [details] patch Clearing flags on attachment: 91263 Committed r85295: <http://trac.webkit.org/changeset/85295>
WebKit Commit Bot
Comment 21 2011-04-28 20:34:09 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 22 2011-04-28 20:53:01 PDT
http://trac.webkit.org/changeset/85295 might have broken Windows 7 Release (Tests)
Note You need to log in before you can comment on or make changes to this bug.