Page MenuHomePhabricator

efl.file_save
Closed, ResolvedPublic

Description

|interface Efl.File_Save
|├ (M) save

Related Objects

StatusAssignedTask
Resolvedzmike
Resolvedzmike
Resolvedzmike
zmike created this task.Jan 31 2019, 5:25 AM
zmike triaged this task as TODO priority.
zmike moved this task from Backlog to Evaluating on the efl: api board.

I do not like Efl.File nor Efl.File_Save, they're extremely tailored to a specific use case:

  • filename AND key?
  • An undocumented flags string which can be "quality=100 compress=9".
  • Efl.File.mmap() requires an Eina.File (so there's Efl.File and Eina.File?)
  • There's save but there's no load.
  • They do not use the Efl.Io classes at all, meaning that we have two orthogonal ways of accessing files.

All in all, these classes look very weird, but I know it is very late in the day to refactor them into using`Efl.Io`.

Can we at least move them to the Efl.Gfx namespace?

zmike added a comment.Jan 31 2019, 9:16 AM

Yeah these are all valid concerns. the file api is very efl-specific: if you're familiar with efl then it's okay but otherwise it's pretty braindamaged as an overall file api. I don't think there are any users of the full file api (e.g., using the key param) which are not image/layout classes, and there are no users of mmap which are not images/layouts.

To propose a more concrete change here:

  • move existing efl_file(_save) into efl_gfx (either directly or as efl.gfx.file)
  • keep simple efl.file mixin with only file property
    • remove key
  • keep simple efl.file_save interface
    • remove flags
  • create file_key property to go with the file api which has been moved to efl_gfx
  • create save_flags property to go with file api in efl_gfx

These changes make sense to me.
Everything would look more integrated if we used Efl.Io instead of file names (we could stream icons from network sockets! awesome!) but that's a lot of work.

The load/save asymmetry bothers me, though. You save to a file, but that's not the file you will retrieve when you read the file property? That's weird.
How about proper save and load methods, accepting a file name (and a key, in the gfx specialization), and a read-only file property to retrieve the last filename used?

cedric added a comment.Feb 1 2019, 3:56 AM

I like @segfaultxavi idea of making load and save a function. Ideally we might want to have an asynchronous version too.

I do not like the idea of having save_flags as a property on the object. It seems weird as it should really be a parameter of the function. What do you think of defining a structure with the two flags we have in it. It can always be expanded later, but we haven't expanded it much over a decade, so it should be fine for some time.

cedric added a comment.Feb 2 2019, 3:43 AM

I actually realized something, if we do move to function for load and save, we end up with no autogenerated way to set the path in a declarative manner like what a MVVM property require. This is not necessarily a problem, but we should be careful on how to address it. To give a bit more context, the idea with MVVM is that the Model will get connected to a View using PropertyBind. PropertyBind take two string, one that correspond to the property in the Model and one that correspond to the property in the View. The call being made on the View, it is the View duty to fetch value from the Model when this value are changed and update the Model when the View itself is altered.

For example if an icon was provided from an edje file, the way to go would be to have one property on the Model that provide both the Path and the Key in one Eina_Value (either with an array of string, or a structure with two strings). The View would have also a property that will accept an Eina_Value that could be either a single string (which case, it is expecting only a Path) or a more complex structure as the one that I am considering here. The View would call the load function and if it gets an error code, it would set that error value on the Model property. If it succeed, it set back the file path and key that was loaded on the Model.

I think the behavior described above make sense for Load. I don't think it would make much sense to be able to connect the Save logic, but I might be wrong.

I understand a file string property is easier to link to MVVM, although there is a lot of hidden knowledge as to when is that property actually read by the widget.
load and save methods are the classical, procedural approach, but they are not as easy to link with a model providing file names.
But you just provided a mechanism to do that, so... there is no problem? I am a bit lost.

I think there isn't a real problem. Just that we will have a specific logic for file load to be connected as a "file" property. I think it is ok as described above that we do not have a symetric behavior for the function and a MVVM property.

Look like we have a plan to move this forward. The only thing missing is a patch now :)

zmike added a comment.Feb 15 2019, 6:08 AM

If someone gives me a tl;dr of what's happening I can take care of this

I initially listed some concerns, which you addressed with specific proposals:

In T7672#130634, @zmike wrote:

To propose a more concrete change here:

  • move existing efl_file(_save) into efl_gfx (either directly or as efl.gfx.file)
  • keep simple efl.file mixin with only file property
    • remove key
  • keep simple efl.file_save interface
    • remove flags
  • create file_key property to go with the file api which has been moved to efl_gfx
  • create save_flags property to go with file api in efl_gfx

And we all agreed, so you are free to implement those.

Afterwards, I also complained (like the tireless little whiner that I am) about the asymmetry of having save() but no load(), and the fact that the filename you save() to is not the same you will retrieve later when reading the file property.
@cedric agreed that conventional load() and save() methods would be nice, but warned us that some work will then be required to link these methods with a filename property on a Model object. I do not know what this work is, but Cedric thinks it is doable.

And this is the TL;DR, as I understand it.

I initially listed some concerns, which you addressed with specific proposals:

In T7672#130634, @zmike wrote:

To propose a more concrete change here:

  • move existing efl_file(_save) into efl_gfx (either directly or as efl.gfx.file)
  • keep simple efl.file mixin with only file property
    • remove key
  • keep simple efl.file_save interface
    • remove flags
  • create file_key property to go with the file api which has been moved to efl_gfx
  • create save_flags property to go with file api in efl_gfx

And we all agreed, so you are free to implement those.

Afterwards, I also complained (like the tireless little whiner that I am) about the asymmetry of having save() but no load(), and the fact that the filename you save() to is not the same you will retrieve later when reading the file property.
@cedric agreed that conventional load() and save() methods would be nice, but warned us that some work will then be required to link these methods with a filename property on a Model object. I do not know what this work is, but Cedric thinks it is doable.

And this is the TL;DR, as I understand it.

I started work on this based on the info here but it seems like we are not all on the same page, so I'm expecting discussions to continue into next week before I take further action.

I think I agree with @segfaultxavi here on most bit:

  • Removing the file and mmap property.
  • Add a load method symmetric to save. I would remove the parameter string for flags, but I would keep the key as it is useful for a lot of archive file format.
  • Add a load_from_file to replace the mmap property.

We could keep the file and mmap get property (not the set), if on save the file property is properly updated and the mmap data is also updated correctly.

For the long term, I would love to have a way to load from an Efl.Io stream as a load function, could be load_from_io then.

Also I am not sure we really need to rename the Efl.File to Efl.Gfx.File. I could imagine that one day someone would use the exact behavior in a non Gfx case.

zmike moved this task from Evaluating to Stabilized on the efl: api board.Feb 27 2019, 12:21 PM
zmike closed this task as Resolved.Mar 11 2019, 10:49 AM
zmike claimed this task.