The way we were considering this was the source defining what it means removing in their context. So, a source could define that remove() means moving to a thrash, while in other case it could mean physically remove it. Also, in those cases that we both options are valid, we were considering using a configuration option, so basically you would were deciding what kind of removing do you want.

But reading again your description, it is clear that this is not enough, because in lot of cases you want different removals in the same source. As example, in the podcasts plugin, in the same session you could use remove() for "mark as readed" or "remove this feed".

So yeah, I can't think on a better approach than the flags. I wonder if it makes sense to have a  predefined set of flags that can be used any source, besides allowing them to add new flags (mostly like we do with metadata keys). So applications that relies on this predefined flags could basically interact with any source without needing to deal with them separately. As saying, not very if it's worth to. I have a mixed feelings.

Created attachment 269926 [details] [review]
WIP: core: Add remove operation flags

Allow passing remove flags to backends to implement various
kinds of removals (deletion, moving to trash, marking as
read/watched, etc.)

I'm feeling we are trying to overload too much the remove() operation.

For instance, the "watched" option. IMHO, "watched" is a mark you want to put in an element when you already have watched that element. Similarly, there's the "favourite" mark. But marking something as "watched" doesn't mean by itself that should be deleted. I think that depends on specific source that could try to give that meaning to elements marked as "watched".

What I'm trying to say? That I think for the case of watched elements, we should use the same approach as marking elements as favourite. That is, the source supports a "watched" key, and when we want to mark something as watched we just set that value to TRUE and call store_metadata() to update the value.

I'm using a Pocket app in my mobile, and that is exactly what it does: when you mark an element as read/watched, it is moved to the archive.

And what it happens with "watched" elements when we do a search/browse? It is clear for me that if we would be deleting them, they should never appear again. But with podcasts or Pocket plugin, while usually we don't want to show the "watched" elements when browsing, sometimes we could want to do it (for instance, to unmark it again). In fact this is what most podcasts apps do: they allow to show all elements or only those not watched.

In our case, I think what to do depends entirely on the source: if it only wants to show unwatched elements, that's fine. Even the source in this case could purge the watched elements from time to time to clear space.

But it could also supports the case of showing all elements, or only the watched ones. If we follow the approach of marking the element as "watched" (the same approach we have used to mark an element as "favourite"), the source could support a filter by "watched" key. So application could get all the elements just by a normal browsing, or only those unwatched using a filter where "watched" == FALSE.

Still, we have the case of "delete" vs. "moving to trash". I'm not sure if repeating the same approach as above is worth or not. That is, we have a "in-trash" key that we set to TRUE when we want to move the element to the thrash. When setting this value, the source internally moves the element to the trash, so it won't be available again in the browse. So all the elements we browse/search have always the "in-trash" value to FALSE. And that somewhat we could access the Trash to "undelete" the elements.

If we still prefer to go with the flags approach, I'm wondering if it would be better to implement as it is proposed, or rather use the GrlCaps/GrlOperationOptions. This latter would allow to introspect what sort of "remove" the source implements, and invoke it with the correct one. Also, it would allow us to extend in the future with other options for the remove() case without breaking the API.

(In reply to comment #3)
> I'm feeling we are trying to overload too much the remove() operation.
> 
> For instance, the "watched" option. IMHO, "watched" is a mark you want to put
> in an element when you already have watched that element. Similarly, there's
> the "favourite" mark. But marking something as "watched" doesn't mean by itself
> that should be deleted. I think that depends on specific source that could try
> to give that meaning to elements marked as "watched".
> 
> What I'm trying to say? That I think for the case of watched elements, we
> should use the same approach as marking elements as favourite. That is, the
> source supports a "watched" key, and when we want to mark something as watched
> we just set that value to TRUE and call store_metadata() to update the value.
> 
> I'm using a Pocket app in my mobile, and that is exactly what it does: when you
> mark an element as read/watched, it is moved to the archive.

The archive isn't accessible through the Pocket plugin in Grilo, so it's as good as removed. The split between watched (read, really) and deleted in Pocket is the same as trashed and deleted (unrecoverably) for local files.

> And what it happens with "watched" elements when we do a search/browse? It is
> clear for me that if we would be deleting them, they should never appear again.

They won't appear again, hence why this is a flag to "remove" rather than simply a property of the media.

> But with podcasts or Pocket plugin, while usually we don't want to show the
> "watched" elements when browsing, sometimes we could want to do it (for
> instance, to unmark it again). In fact this is what most podcasts apps do: they
> allow to show all elements or only those not watched.

We don't show items that are in the trash. For a similar reason, we don't offer a UI to Pocket's archives articles/videos.

> In our case, I think what to do depends entirely on the source: if it only
> wants to show unwatched elements, that's fine. Even the source in this case
> could purge the watched elements from time to time to clear space.
> 
> But it could also supports the case of showing all elements, or only the
> watched ones. If we follow the approach of marking the element as "watched"
> (the same approach we have used to mark an element as "favourite"), the source
> could support a filter by "watched" key. So application could get all the
> elements just by a normal browsing, or only those unwatched using a filter
> where "watched" == FALSE.
> 
> Still, we have the case of "delete" vs. "moving to trash". I'm not sure if
> repeating the same approach as above is worth or not. That is, we have a
> "in-trash" key that we set to TRUE when we want to move the element to the
> thrash. When setting this value, the source internally moves the element to the
> trash, so it won't be available again in the browse. So all the elements we
> browse/search have always the "in-trash" value to FALSE. And that somewhat we
> could access the Trash to "undelete" the elements.

This is really weird. I don't think that changing a property on a media should make it unavailable.

> If we still prefer to go with the flags approach, I'm wondering if it would be
> better to implement as it is proposed, or rather use the
> GrlCaps/GrlOperationOptions. This latter would allow to introspect what sort of
> "remove" the source implements, and invoke it with the correct one. Also, it
> would allow us to extend in the future with other options for the remove() case
> without breaking the API.

I think this is the way I'll go then.

(In reply to comment #4)
> (In reply to comment #3)
> > I'm using a Pocket app in my mobile, and that is exactly what it does: when you
> > mark an element as read/watched, it is moved to the archive.
> 
> The archive isn't accessible through the Pocket plugin in Grilo, so it's as
> good as removed. The split between watched (read, really) and deleted in Pocket
> is the same as trashed and deleted (unrecoverably) for local files.
> 

But this is due the way you have implemented the Pocket plugin in Grilo: you just don't want to show watched elements, and don't have the option to allow application to get them. But if you think in a more general approach, I don't think that a "watched" element should always be removed. As I explained in the case of the Podcasts, a plugin that allow to list all content or only unwatched content is a very common case.

And even when it seems that marking as watched and delete could seem the same (from the Pocket plugin point of view) it is not, because if you access the Pocket service from your browser, the result would be very different if you have deleted or archive.

> > And what it happens with "watched" elements when we do a search/browse? It is
> > clear for me that if we would be deleting them, they should never appear again.
> 
> They won't appear again, hence why this is a flag to "remove" rather than
> simply a property of the media.
> 

Maybe the point here is about the interpretation of what it means "archive" in the case of Pocket. For me, archive means "this is read/watched, keep it for the future". From what I see, for you it means "delete this element by putting it in the archive (like a Thrash)".


> > But with podcasts or Pocket plugin, while usually we don't want to show the
> > "watched" elements when browsing, sometimes we could want to do it (for
> > instance, to unmark it again). In fact this is what most podcasts apps do: they
> > allow to show all elements or only those not watched.
> 
> We don't show items that are in the trash. For a similar reason, we don't offer
> a UI to Pocket's archives articles/videos.
> 

Would it make sense to show them? I really think that the archive is the place to store things already watched, and that you want to keep them for the future, because they are interesting.

> > In our case, I think what to do depends entirely on the source: if it only
> > wants to show unwatched elements, that's fine. Even the source in this case
> > could purge the watched elements from time to time to clear space.
> > 
> > But it could also supports the case of showing all elements, or only the
> > watched ones. If we follow the approach of marking the element as "watched"
> > (the same approach we have used to mark an element as "favourite"), the source
> > could support a filter by "watched" key. So application could get all the
> > elements just by a normal browsing, or only those unwatched using a filter
> > where "watched" == FALSE.
> > 
> > Still, we have the case of "delete" vs. "moving to trash". I'm not sure if
> > repeating the same approach as above is worth or not. That is, we have a
> > "in-trash" key that we set to TRUE when we want to move the element to the
> > thrash. When setting this value, the source internally moves the element to the
> > trash, so it won't be available again in the browse. So all the elements we
> > browse/search have always the "in-trash" value to FALSE. And that somewhat we
> > could access the Trash to "undelete" the elements.
> 
> This is really weird. I don't think that changing a property on a media should
> make it unavailable.
> 

I agree that implementing this with a key could be weird. But I don't think that it is weird to hide elements are depending on the value of a key. This is what I'm proposing with the case of "watched" elements.


> > If we still prefer to go with the flags approach, I'm wondering if it would be
> > better to implement as it is proposed, or rather use the
> > GrlCaps/GrlOperationOptions. This latter would allow to introspect what sort of
> > "remove" the source implements, and invoke it with the correct one. Also, it
> > would allow us to extend in the future with other options for the remove() case
> > without breaking the API.
> 
> I think this is the way I'll go then.

Nice.

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > I'm using a Pocket app in my mobile, and that is exactly what it does: when you
> > > mark an element as read/watched, it is moved to the archive.
> > 
> > The archive isn't accessible through the Pocket plugin in Grilo, so it's as
> > good as removed. The split between watched (read, really) and deleted in Pocket
> > is the same as trashed and deleted (unrecoverably) for local files.
> > 
> 
> But this is due the way you have implemented the Pocket plugin in Grilo: you
> just don't want to show watched elements, and don't have the option to allow
> application to get them. But if you think in a more general approach, I don't
> think that a "watched" element should always be removed. As I explained in the
> case of the Podcasts, a plugin that allow to list all content or only unwatched
> content is a very common case.
> 
> And even when it seems that marking as watched and delete could seem the same
> (from the Pocket plugin point of view) it is not, because if you access the
> Pocket service from your browser, the result would be very different if you
> have deleted or archive.

For the podcast plugin, you'd use metadata on those files. You wouldn't remove things you mark as watched.

> > > And what it happens with "watched" elements when we do a search/browse? It is
> > > clear for me that if we would be deleting them, they should never appear again.
> > 
> > They won't appear again, hence why this is a flag to "remove" rather than
> > simply a property of the media.
> > 
> 
> Maybe the point here is about the interpretation of what it means "archive" in
> the case of Pocket. For me, archive means "this is read/watched, keep it for
> the future". From what I see, for you it means "delete this element by putting
> it in the archive (like a Thrash)".

I can rename it "archive" if that's clearer. It makes no difference to me.

> > > But with podcasts or Pocket plugin, while usually we don't want to show the
> > > "watched" elements when browsing, sometimes we could want to do it (for
> > > instance, to unmark it again). In fact this is what most podcasts apps do: they
> > > allow to show all elements or only those not watched.
> > 
> > We don't show items that are in the trash. For a similar reason, we don't offer
> > a UI to Pocket's archives articles/videos.
> > 
> 
> Would it make sense to show them? I really think that the archive is the place
> to store things already watched, and that you want to keep them for the future,
> because they are interesting.

No, not in grilo anyway. I'd make it accessible if there was a separate application for all things Pocket though.

> > > In our case, I think what to do depends entirely on the source: if it only
> > > wants to show unwatched elements, that's fine. Even the source in this case
> > > could purge the watched elements from time to time to clear space.
> > > 
> > > But it could also supports the case of showing all elements, or only the
> > > watched ones. If we follow the approach of marking the element as "watched"
> > > (the same approach we have used to mark an element as "favourite"), the source
> > > could support a filter by "watched" key. So application could get all the
> > > elements just by a normal browsing, or only those unwatched using a filter
> > > where "watched" == FALSE.
> > > 
> > > Still, we have the case of "delete" vs. "moving to trash". I'm not sure if
> > > repeating the same approach as above is worth or not. That is, we have a
> > > "in-trash" key that we set to TRUE when we want to move the element to the
> > > thrash. When setting this value, the source internally moves the element to the
> > > trash, so it won't be available again in the browse. So all the elements we
> > > browse/search have always the "in-trash" value to FALSE. And that somewhat we
> > > could access the Trash to "undelete" the elements.
> > 
> > This is really weird. I don't think that changing a property on a media should
> > make it unavailable.
> > 
> 
> I agree that implementing this with a key could be weird. But I don't think
> that it is weird to hide elements are depending on the value of a key. This is
> what I'm proposing with the case of "watched" elements.

In the case of Pocket, it would disappear from the "unread" view completely, our only view. I would use a function to enact an action, I wouldn't set a property.

(In reply to comment #6)
> > And even when it seems that marking as watched and delete could seem the same
> > (from the Pocket plugin point of view) it is not, because if you access the
> > Pocket service from your browser, the result would be very different if you
> > have deleted or archive.
> 
> For the podcast plugin, you'd use metadata on those files. You wouldn't remove
> things you mark as watched.
> 

I don't know what you mean by "use metadata on those files". 

I think the difference between your pov and mine is about what it means in Pocket to move to archive: in your pov, it means like moving to Trash, in mine it means like marking something as watched (correct me if I'm wrong).

> > > > And what it happens with "watched" elements when we do a search/browse? It is
> > > > clear for me that if we would be deleting them, they should never appear again.
> > > 
> > > They won't appear again, hence why this is a flag to "remove" rather than
> > > simply a property of the media.
> > > 
> > 
> > Maybe the point here is about the interpretation of what it means "archive" in
> > the case of Pocket. For me, archive means "this is read/watched, keep it for
> > the future". From what I see, for you it means "delete this element by putting
> > it in the archive (like a Thrash)".
> 
> I can rename it "archive" if that's clearer. It makes no difference to me.

Neither for me. It is not about the name, but about what's the meaning.

> > Would it make sense to show them? I really think that the archive is the place
> > to store things already watched, and that you want to keep them for the future,
> > because they are interesting.
> 
> No, not in grilo anyway. I'd make it accessible if there was a separate
> application for all things Pocket though.
> 

Fair enough.

> > I agree that implementing this with a key could be weird. But I don't think
> > that it is weird to hide elements are depending on the value of a key. This is
> > what I'm proposing with the case of "watched" elements.
> 
> In the case of Pocket, it would disappear from the "unread" view completely,
> our only view. I would use a function to enact an action, I wouldn't set a
> property.

Yes. I guess that the unread view is the one we want to show in our app. But "all" view is something that could be interesting for other apps. I don't mean we need to do it, though. The point is when we are going to add a new feature, try to generalize as much as possible so most apps/sources can take advantage of that feature.

Created attachment 276409 [details] [review]
core: Rename GrlOperationOptions "flags" to "resolution-flags"

And rename the associated functions as well. This will allow us
to add new Flags without trampling on those ones.

Created attachment 276410 [details] [review]
core: Add remove operation flags

Allow passing remove flags to backends to implement various
kinds of removals (deletion, moving to trash, marking as
read/watched, etc.)

Created attachment 276411 [details] [review]
test-ui: Don't remove items from the view when remove fails

Created attachment 276412 [details] [review]
all: Port from ..._flags() to ..._resolution_flags()

It should be possible to skip the "Rename flags to resolution-flags" patch, and move the additional struct member of GrlSourceRemoveSpec at the end. This might be enough to avoid breaking ABI/API.

(In reply to comment #12)
> It should be possible to skip the "Rename flags to resolution-flags" patch, and
> move the additional struct member of GrlSourceRemoveSpec at the end. This might
> be enough to avoid breaking ABI/API.

And obviously create new "with_options" versions of grl_source_remove*

Created attachment 276441 [details] [review]
core: Add remove operation flags

Allow passing remove flags to backends to implement various
kinds of removals (deletion, moving to trash, marking as
read/watched, etc.)

Created attachment 276442 [details] [review]
core: Rename GrlOperationOptions "flags" to "resolution-flags"

And add the associated functions as well. Deprecate the old
functions.

Created attachment 276443 [details] [review]
core: Add remove operation flags

Allow passing remove flags to backends to implement various
kinds of removals (deletion, moving to trash, marking as
read/watched, etc.)

Created attachment 276444 [details] [review]
core: Rename GrlOperationOptions "flags" to "resolution-flags"

And add the associated functions as well. Deprecate the old
functions.

Created attachment 277556 [details] [review]
core: Add remove operation flags

Allow passing remove flags to backends to implement various
kinds of removals (deletion, moving to trash, marking as
read/watched, etc.)

Created attachment 277557 [details] [review]
core: Rename GrlOperationOptions "flags" to "resolution-flags"

And add the associated functions as well. Deprecate the old
functions.

Created attachment 277558 [details] [review]
core: Fix possible crash when passing NULL options

grl_operation_options_get_resolution_flags() shouldn't
crash when the options passed are NULL.

Attachment 277557 [details] pushed as c65c68b - core: Rename GrlOperationOptions "flags" to "resolution-flags"
Attachment 277558 [details] pushed as 3ba699d - core: Fix possible crash when passing NULL options

Comment on attachment 276412 [details] [review]
all: Port from ..._flags() to ..._resolution_flags()

Was committed as 878a0bf600eab88fde31453b8933d705b65e2ad0

Comment on attachment 276411 [details] [review]
test-ui: Don't remove items from the view when remove fails

Committed in 37e40deab108bffc58c80627ff9f76ef2b73b7cb

Created attachment 354689 [details] [review]
core: Add remove operation flags

Allow passing remove flags to backends to implement various
kinds of removals (deletion, moving to trash, marking as
read/watched, etc.)

Moved to https://gitlab.gnome.org/GNOME/grilo/merge_requests/11