Mark comments as read

[Pytal]

Fix #23877

Calls the existing DAV comments read handler

server/apps/dav/lib/Comments/EntityCollection.php

Lines 179 to 181 in 529d653

	public function propPatch(PropPatch $propPatch) {
	$propPatch->handle(self::PROPERTY_NAME_READ_MARKER, [$this, 'setReadMarker']);
	}

Run the following SQL query to observe updates to the database

select *
from oc_comments_read_markers
order by marker_datetime

[skjnldsv]

There seems to be much more than just the comments as read?
Why can't we just watch the Comments component and handle read on scroll/focus or something? I am not super fond of this API change for comments only? Or maybe there are other usages this æctive handler might have on other tabs?

[Pytal]

The active handler, essentially the focus handler, seemed appropriate and would fit alongside the existing mount, update, and destroy handler API, although it may not be worth it if it's only used in comments

There are two ways then

1. Add the active handler to the sidebar tab API which could also be used by other tabs
2. Add https://github.com/Akryum/vue-observe-visibility as a dependency and do something like

diff --git a/apps/comments/src/services/CommentsInstance.js b/apps/comments/src/services/CommentsInstance.js
index f5263f35c3..4a9c90d220 100644
--- a/apps/comments/src/services/CommentsInstance.js
+++ b/apps/comments/src/services/CommentsInstance.js
@@ -24,6 +24,7 @@ import { getLoggerBuilder } from '@nextcloud/logger'
 import { translate as t, translatePlural as n } from '@nextcloud/l10n'
 import CommentsApp from '../views/Comments'
 import Vue from 'vue'
+import VueObserveVisibility from 'vue-observe-visibility'
 
 const logger = getLoggerBuilder()
        .setApp('comments')
@@ -61,6 +62,8 @@ export default class CommentInstance {
                        },
                })
 
+               Vue.use(VueObserveVisibility)
+
                // Init Comments component
                const View = Vue.extend(CommentsApp)
                return new View(options)
diff --git a/apps/comments/src/views/Comments.vue b/apps/comments/src/views/Comments.vue
index fbe81d23b9..65e77c8f54 100644
--- a/apps/comments/src/views/Comments.vue
+++ b/apps/comments/src/views/Comments.vue
@@ -21,7 +21,9 @@
   -->
 
 <template>
-       <div class="comments" :class="{ 'icon-loading': isFirstLoading }">
+       <div class="comments"
+               :class="{ 'icon-loading': isFirstLoading }"
+               v-observe-visibility="onVisibilityChange">
                <!-- Editor -->
                <Comment v-bind="editorData"
                        :auto-complete="autoComplete"
@@ -127,6 +129,17 @@ export default {
        },
 
        methods: {
+               async onVisibilityChange(isVisible) {
+                       if (isVisible) {
+                               try {
+                                       await markCommentsAsRead(this.commentsType, this.ressourceId, new Date())
+                                       emit('comments:comments:read', { ressourceId: this.ressourceId })
+                               } catch (e) {
+                                       showError(e.message || t('comments', 'Error marking comments as read'))
+                               }
+                       }
+               },
+
                /**
                 * Update current ressourceId and fetch new data
                 *

What do you think @skjnldsv?

[Pytal]

What do you think about the active vs. observability approaches above @artonge?

[skjnldsv]

Sorry, I forgot t reply.
My concerns is aboutover-engineering this.

Do we actually require to check for focus?
Can't we just assume that the activeTab being set to true means the tab is displayed and the comment read?

[artonge]

What do you think about the active vs. observability approaches above @ artonge?

I don't know, what is Talk doing to check if a message is read? Maybe we can reuse the logic? In any case I would choose what makes a consistent feeling.
For me, it is more a UX question than an engineering one.

[Pytal]

Do we actually require to check for focus? Can't we just assume that the activeTab being set to true means the tab is displayed and the comment read?

The active handler is handling the "activeTab being set to true", if there is somewhere else in the code to handle "activeTab being set to true" cleanly without the API addition I cannot grep it currently and so I am open to any suggestions @skjnldsv

The hacky way without the API addition would be to watch activeTab in https://github.com/nextcloud/server/blob/8b23c1c6772afdd4a1709bdb85cf3b88c5b5a6a5/apps/files/src/views/Sidebar.vue and call markCommentsAsRead() if activeTab === 'comments' which is outside the scoped sidebar tab API system and therefore not a viable solution

I don't know, what is Talk doing to check if a message is read? Maybe we can reuse the logic? In any case I would choose what makes a consistent feeling.
For me, it is more a UX question than an engineering one.

Talk is invoking it on scroll, see https://github.com/nextcloud/spreed/blob/d9b7a3f53bb40fadc0295a52b4e0f52605e179f5/src/components/MessagesList/MessagesList.vue#L421-L424 -> https://github.com/nextcloud/spreed/blob/d9b7a3f53bb40fadc0295a52b4e0f52605e179f5/src/components/MessagesList/MessagesList.vue#L694-L757, which would only add complexity compared to the boolean check of marking comments as read on active

The implementation of the active handler and other higher up changes in the current state of the PR was driven by responsiveness requirements for UX so that the unread comments icon disappears when comments are read instead of having to reload the page

[marcelklehr]

Hello everyone,

I was assigned maintainer of the comments app recently.

@Pytal I think going forward with the visbilty observer is the best option!

[Pytal]

/compile amend /

[marcelklehr]

/rebase

[marcelklehr]

/compile

[marcelklehr]

/compile

[miaulalala]

LGTM didn't test but PROPPATCH looks fine

Outdated