MediaPlayer: Mark as read on audio/video completion #2684
Conversation
ztec
force-pushed
the
markAsReadWhenListened
branch
2 times, most recently
from
41626fa
to
5d2621b
Compare
|
The PR is ready for review. As mentioned by @rdelaage in the issue #1847 (comment), Maybe this option should be complemented by a feed override option. Depending of how the global option is welcomed and how people use it I can implement it. |
| @@ -114,6 +114,7 @@ <h1 id="page-header-title">{{ t "page.settings.title" }}</h1> | |||
| <label><input type="checkbox" name="show_reading_time" value="1" {{ if .form.ShowReadingTime }}checked{{ end }}> {{ t "form.prefs.label.show_reading_time" }}</label> | |||
|
|
|||
| <label><input type="checkbox" name="mark_read_on_view" value="1" {{ if .form.MarkReadOnView }}checked{{ end }}> {{ t "form.prefs.label.mark_read_on_view" }}</label> | |||
| <label>↳ <input type="checkbox" name="mark_read_on_enclosure_completion" value="1" {{ if .form.MarkReadOnEnclosureCompletion }}checked{{ end }}> {{ t "form.prefs.label.mark_read_on_EnclosureCompletion" }}</label> | |||
There was a problem hiding this comment.
The ↳ is here to show that this options modify the previous option's behavior. Initially it was directly included in the wording but I found it a bit misleading and reworded it.
The two options are independently enablable but the second will affect the first when enabled (actually disabling it when audio/video enclosures are present)
Of course I can remove the arrow, you decide in the end. I just feel there is something missing without it and I don't have any better proposition that would not require a complete paragraph to explain properly the option in the UI.
Are you sure you want to remove it ?
There was a problem hiding this comment.
My main concern is from an accessibility point of view. I don't know how ↳ will be interpreted by screen readers. Also, from a usability perspective it might confuse some users.
Perhaps, the web ui could be changed to a list of choices to make things more explicit:
The logic to process the form might be more complex, but at least no additional Javascript is required.
There was a problem hiding this comment.
I will try to experiment and see If I can implement something like you suggest.
There was a problem hiding this comment.
My last commit (df849fc) contains a proposition. Can you have a look and tell me if it is ok for you ?
The commit is not mergable as is, I'm just asking for a global opinion on the way I did it. If it feel ok for you,
I will polish the changes and integrate the missing i18n translations. If you have any remarks, I will work on them.
As I stated in the commit , few tweaks may be added (CSS). Also the final options text is open to better suggestions. the current proposition contains the necessary information in my opinion but may clearly be improved.
The actual result from the web:
I tested the setting form and each 4 options on the undread entry pages.
There was a problem hiding this comment.
Looks okay to me, but I would reword the text because the sentence is quite long. Maybe something more concise like this:
- Automatically mark entries as read when viewed
- Mark entries as read when viewed. For audio/video, mark as read at 90% completion
- Only mark as read when audio/video playback reaches 90% completion
- Mark entries as read manually
There was a problem hiding this comment.
I've put your suggestion into the form and setup the i18n accordingly. The Length is still an issue in the literal french translation I made tho. Not a big deal I think.
|
I've re-tested on firefox-linux all the usecase I listed previously after the changes. |
ztec
force-pushed
the
markAsReadWhenListened
branch
from
dd9c540
to
df849fc
Compare
When listening to enclosure media like Podcasts or Peertube feeds, you now have an option to mark the entry as read when the playback reach 90% instead of right after viewing the entry page. In order to do that, I had to modify every place where the MarkAsReadOnView decision was made. To centralize the new condition I moved it in the `entry` entity. It creates a coupling with the `user` model. I could have put it in the user model but the coupling would be the same. I'm not aware of a more suitable place to put this kind of small code (such as an `utils` module) If the new option is enabled, the existing option `mark as read on view` will only trigger if the entry has no audio or video enclosure at all. Image enclosure are excluded because they are not relevant for the feature in my opinion. I've renamed the `handlePlayerProgressionSave` to accommodate the new behavior. I could have created another function but this would have duplicate some code, I felt it is better to just add the new behavior in the end. Debatable of course. In order to `mark as read` I reused the `handleEntryStatus` function. This has the advantage to show the status change on the `unread/read` button on top. The user model have a new setting, and I've added a new DB migration to add the field as well. The completion trigger is defined by the `data-mark-read-on-completion` value. It is a percentage (from 0 to 1). For now, I've hardcoded the value to 0.9 => 90%, but maybe we will feel the need to make it configurable at some point. (on a by feed basis) I've tested the feature on - firefox - linux - vivaldi - android I've included English and French i18n version. I've checked the `mark as read` button and player behavior on - unread entry pages - search entry pages - categories entry pages - history entry pages - unread list - categories list - history pages
ztec
force-pushed
the
markAsReadWhenListened
branch
from
066b264
to
65487c8
Compare
|
I've rebase and all, re-did the tests I've mentioned in my initial comment. All seems ok |
When listening to enclosure media like Podcasts or Peertube feeds, you now have an option to mark the entry as read when the playback reach 90% instead of right after viewing the entry page.
In order to do that, I had to modify every place where the MarkAsReadOnView decision was made. To centralize the new condition I moved it in the
entryentity. It create a coupling with theusermodel. I could have put it in the user model but the coupling would be the same. I'm not aware of a more suitable place to put this kind of small code (such as anutilsmodule)If the new option is enabled, the existing option
mark as read on viewwill only trigger if the entry has no audio or video enclosure at all. Image enclosure are excluded because they are not relevant for the feature in my opinon.I've rename the
handlePlayerProgressionSaveto accommodate the new behavior. I could have created another function but this would have duplicate some code, I felt it is better to just add the new behavior in the end. Debatable of course. In order tomark as readI reused thehandleEntryStatusfunction. This has the advantage to show the status change on theunread/readbutton on top.The user model have a new setting, and I've added a new DB migration to add the field as well.
The completion trigger is defined by the
data-mark-read-on-completionvalue. It is a percentage (from 0 to 1).For now I've hardcoded the value to 0.9 => 90% but maybe we will feel the need to make it configurable at some point. (on a by feed basis)
I've tested the feature on
I've checked the
mark as readbutton and player behavior onTODO before getting the PR out of Draft
Do you follow the guidelines?