Mark comments as read

[Pytal]

Fix https://github.com/nextcloud/server/issues/23877

Calls the existing DAV comments read handler https://github.com/nextcloud/server/blob/529d6537b25c3d7068eedbeb4b88797fe83a4e71/apps/dav/lib/Comments/EntityCollection.php#L179-L181

Run the following SQL query to observe updates to the database

select *
from oc_comments_read_markers
order by marker_datetime

[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

[PVince81]

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.

In Talk it's quite complex and cumbersome (was hard to implement accurately, I believe there are still bugs there) We check what element is visible at the top of the window (some calculation on scrollTop, etc) and then mark this one as unread, unless the container is scrolled to bottom in which case we mark the whole conversation as read. And not only that, the initial panel's scroll needed to be automatically scrolled to the first unread message. Ref: https://github.com/nextcloud/spreed/blob/master/src/components/MessagesList/MessagesList.vue#L692

I'd advise to not go to such lengths considering that the "Comments" tab is much less used than Talk, so a simple approach is better for now.

[Pytal]

Using intersection observer seems simpler and requires less code than adding the active handler boilerplate, so would opt for that

2. Add 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
                 *

Re:

Can't we just assume that the activeTab being set to true means the tab is displayed and the comment read?

Using activeTab would require polluting the shared sidebar tab code with comments tab specific code, to pass down the active state to the comments tab itself we would need the active handler boilerplate as added previously

The Intersection Observer API is also supported by our target browsers going by the compatibility table https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API#browser_compatibility and our browserslist config https://browserslist.dev/?q=PjAuMjUlLCBub3QgaWUgMTEsIG5vdCBvcF9taW5pIGFsbCwgbm90IGRlYWQsIEZpcmVmb3ggRVNS

What do you think @skjnldsv @artonge @PVince81?

[PVince81]

the code in Talk I pointed at is also just a part of it, it does rely on a class that is set by the "observe visibility" clause, see https://github.com/nextcloud/spreed/blob/master/src/components/MessagesList/MessagesGroup/Message/Message.vue#L186

[Pytal]

the code in Talk I pointed at is also just a part of it, it does rely on a class that is set by the "observe visibility" clause, see https://github.com/nextcloud/spreed/blob/master/src/components/MessagesList/MessagesGroup/Message/Message.vue#L186

The comments read marker in the database is just a timestamp for when the comments of the file/folder were last viewed without the specificity of which comment was read last unlike in Talk which uses the token and message id, so only the code in the diff posted above would be needed for comments implementation without the complexity of the Talk code

[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 /

[Pytal]

/compile amend /

[marcelklehr]

/rebase

[marcelklehr]

/compile

[marcelklehr]

/compile