Page MenuHomePhabricator

Add new Efl.Filter.String Class
AbandonedPublic

Authored by Hermet on Jun 14 2017, 10:56 PM.

Details

Summary

This allows you to apply own tags to original data before copy and paste are finished.

Test Plan
  1. run elementary_test -to EflUiTextFilter
  2. press 'Add Copy Filter' button and copy some text in the entry
  3. paste copied data to the entry
  4. you can see that original copied data is changed by copy filter
  5. press 'Delete Filters' button to remove copy filter
  6. copy some text in the entry and paste again
  7. you can see original copied data is not changed
  8. press 'Add Paste Filter' button and paste copied data to the entry
  9. you can see original pasted data is changed by paste filter.

Diff Detail

Repository
rEFL core/efl
Branch
arcpatch-D4966
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3975
Build 4040: arc lint + arc unit
herb created this revision.Jun 14 2017, 10:56 PM
herb updated this revision to Diff 11610.Jun 14 2017, 11:13 PM

remove unused warning

jpeg requested changes to this revision.Jun 14 2017, 11:49 PM
  1. The function prototypes are not good. Just return a char *, don't use an out value.
  2. I think the filter should be a separate obejct (delegate mode), of an interface "Efl.Filter.String" for instance, that would have a single function:
class Efl.Filter.String {
   methods {
      filter_string {
         params {
            @in in: string;
         }
         return: own(string); [[By default returns strdup(string).]]
      }
   }
}

Then we can install a filter with function like entry.filter_set(COPY | PASTE, my_filter_object):

Efl.Ui.Text { @property filter { keys { copy_or_paste_or_input_whatever; } values { filter: Efl.Filter.String; } }

Some people may not like the use of a delegate object, that's for sure. But it can have its advantages: no need to always call the filter function if there is no filter installed for this operation.

  1. Please simplify the code. We have eina_slstr, eina_strbuf, etc... There are no null checks where there should have been, etc...
  2. The return value could be a const char *, if we encourage using eina_slstr for this.
src/bin/elementary/test_markup_filter.c
24 ↗(On Diff #11610)

Please use eina_strbuf in this code. It will be much simpler :)

36 ↗(On Diff #11610)

eina_strbuf

src/lib/efl/interfaces/efl_markup_filter.eo
5 ↗(On Diff #11610)

It's an interface, pure_virtual is not necessary.

12 ↗(On Diff #11610)

why not return??

15 ↗(On Diff #11610)

same here

22 ↗(On Diff #11610)

why not return??
return: own(string)

26 ↗(On Diff #11610)

I think the names copy & paste are too simple and generic here. Also they don't match much with the name of the class here.

src/lib/elementary/elm_entry.c
662 ↗(On Diff #11610)

why is this not a return value?

678 ↗(On Diff #11610)

strdup????
i don't even see why there is a string copy here...

src/lib/elementary/elm_entry.eo
991 ↗(On Diff #11610)

elm::Entry txt;
string copied = txt.copy("hello");
string pasted = txt.paste("hi");

This revision now requires changes to proceed.Jun 14 2017, 11:49 PM
herb updated this revision to Diff 11650.Jun 19 2017, 3:31 AM
herb edited edge metadata.

Add new Efl.Filter.String Class

herb updated this revision to Diff 11651.Jun 19 2017, 4:27 AM

Add new Efl.Fitter.String class

herb updated this revision to Diff 11652.Jun 19 2017, 4:37 AM

remove warning

herb retitled this revision from Add new Efl_Markup_Filter interface to Add new Efl.Filter.String Class.Jun 19 2017, 4:39 AM
herb edited the test plan for this revision. (Show Details)
jpeg requested changes to this revision.Jun 20 2017, 10:59 PM
jpeg added a subscriber: herdsman.

But please hold on to this for now.

I do not like the fact that we need strdup at the very least. I think there should not be any allocation of string if that's possible.
In C, this is an issue, we can't just return string and let the language handle the references. We have eina_stringshare but that would be overkill here and stringshare allocates memory anyway.

I actually wonder if we couldn't use eina_slstr for strings that need allocation. So, the EO signature would be return: string; i.e. const char *.
The default implementation would be:

const char *
default_filter(Eo * EINA_UNUSED, const char *in)
{
   return in;
}

While a user implementation could do:

const char *
custom_filter(Eo * EINA_UNUSED, const char *in)
{
   return eina_slstr_printf("INPUT: %s", in);
}

Last, but not least, there was a recent patch about using function pointers in EO. If we move forward with function pointers then this API will not require a class, only a Function. The same remark about the lifecycle of the string remains valid.

I'd like @cedric's comment on it.

PS: If we decide to use eina_slstr extensively for this kind of feature, we will need an extra API to steal a char * from an eina_slstr * (it's doable).

src/bin/elementary/test_efl_ui_text.c
286

replace the above 3 lines with:

return eina_strbuf_release(buf);

:-)

298

see above

src/lib/efl/interfaces/efl_filter_string.c
13

missing EINA_UNUSED - please fix all warnings

src/lib/elementary/efl_ui_text.c
1486

As you can see, there's an extra free() here. Cumbersome I believe.

5412

del + add can be avoided if the filter isn't changed.

src/lib/elementary/efl_ui_text.eo
62

invalid: Efl.Ui.Text is not a filter itself

291

I believe this should be part of a CnP or maybe generic Text interface. @herdsman ?

This revision now requires changes to proceed.Jun 20 2017, 10:59 PM
raster edited edge metadata.Jun 20 2017, 11:17 PM

 We have eina_stringshare but that would be overkill here and stringshare allocates memory anyway.

This would only be useful if the string is in fact used exactly the same in multiple places. Not here... so if you dont intend to make any changes you SHOULD be able to just return the input string as given. That means the input string passed into your filter needs to be:

  • allocated by strdup/malloc etc.
  • freed with free() when done with by the caller of this filter.

We need this anyway to free up the return (we need to know how to) so at least a "I decided to do nothing" means just returning what was passed in. If you do change then free your input string once the new one is generated and return the alloced new string... :) in bindings i can assume bindings + ownership tags should take care of this.

use eina_slstr for strings

This would be maybe a good option... :)

jpeg added a comment.Jun 20 2017, 11:27 PM

@raster I shouldn't have mentionned stringshare :)
eina_slstr is the only option I was actually considering here.

herdsman added inline comments.Jun 21 2017, 2:13 AM
src/lib/elementary/efl_ui_text.eo
291

Well, we need to write the Efl.Cnp interface, then.
It's part of the plan for our interface work (unless something had changed), so maybe we can wait with this patch until then.

Also, is there a reason that efl_filter_string.eo still resides in "src/efl/interfaces"? Same thing for the doc: "Efl filter string interface".
It feels more like a utility object. If it only serves Copy-and-paste, you can put it in Efl.Ui.
Maybe we want the following flow:


// Efl.Ui.Text implements Efl.Cnp, specifically Efl.Cnp.filter_create
Efl_Filter_String *filter = efl_cnp_filter_create(ui_text);

EFL_OPS_DEFINE(custom_filter_ops,
               EFL_OBJECT_OP_FUNC(efl_filter_string, _custom_filter_copy));
efl_object_override(filter, &custom_filter_ops);
jpeg added a comment.Jun 21 2017, 2:42 AM

Also, is there a reason that efl_filter_string.eo still resides in "src/efl/interfaces"? Same thing for the doc: "Efl filter string interface".
It feels more like a utility object. If it only serves Copy-and-paste, you can put it in Efl.Ui.
Maybe we want the following flow:

 
// Efl.Ui.Text implements Efl.Cnp, specifically Efl.Cnp.filter_create
Efl_Filter_String *filter = efl_cnp_filter_create(ui_text);
 
EFL_OPS_DEFINE(custom_filter_ops,
               EFL_OBJECT_OP_FUNC(efl_filter_string, _custom_filter_copy));
efl_object_override(filter, &custom_filter_ops);

The problem above is that the filter is not unique to the entry ui_text. So its lifecycle is undefined.

I agree that Efl.Filter.String doesn't need to be in efl/interfaces/. It needs to be wherever our CnP, DnD and text UI classes are. So elementary is probably good enough.

src/lib/elementary/efl_ui_text.eo
291

Yes I guess @thiepha can merge this function while working on the cnp API. No hurry.

herb added inline comments.Jun 22 2017, 12:13 AM
src/lib/elementary/efl_ui_text.eo
291

I will merge this patch to elm_cnp interface when the interface work is finished :)

jpeg added a comment.Sep 19 2017, 3:13 AM

Since we now have function pointers I think this can be simplified a lot.
Where are we on the CnP and other interfaces?

Hermet commandeered this revision.Mar 27 2019, 6:46 PM
Hermet added a reviewer: herb.
Hermet abandoned this revision.Mar 27 2019, 6:46 PM

This is a too old patch, nobody keeps tracking on this anymore.