From 8fc811dfa92a7212bcf7e807f95603ea82fe19bd Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Wed, 4 Aug 2021 22:59:03 +0000 Subject: [PATCH] unread: Add UI support for marking messages as unread. Fixes #2676. Co-authored-by: Aman Agrawal Co-authored-by: Tim Abbott --- frontend_tests/node_tests/hotkey.js | 2 +- frontend_tests/node_tests/message_flags.js | 21 ++++++++++++++++++ frontend_tests/node_tests/popovers.js | 5 +++++ static/js/hotkey.js | 5 +++++ static/js/message_flags.js | 4 ++++ static/js/message_list.js | 22 ++++++++++++++++++- static/js/message_list_data.js | 15 +++++++++++++ static/js/popovers.js | 23 ++++++++++++++++++++ static/js/unread_ops.js | 21 ++++++++++++++++++ static/js/unread_ui.js | 2 ++ static/styles/dark_theme.css | 5 ++++- static/styles/popovers.css | 22 +++++++++++++++++++ static/templates/actions_popover_content.hbs | 10 +++++++++ static/templates/keyboard_shortcuts.hbs | 4 ++++ templates/zerver/help/keyboard-shortcuts.md | 2 ++ 15 files changed, 160 insertions(+), 3 deletions(-) diff --git a/frontend_tests/node_tests/hotkey.js b/frontend_tests/node_tests/hotkey.js index 3f7e820019..8f9350f42d 100644 --- a/frontend_tests/node_tests/hotkey.js +++ b/frontend_tests/node_tests/hotkey.js @@ -262,7 +262,7 @@ run_test("allow normal typing when processing text", ({override, override_rewire // Unmapped keys should immediately return false, without // calling any functions outside of hotkey.js. assert_unmapped("bfmoyz"); - assert_unmapped("BEFHILNOQTUWXYZ"); + assert_unmapped("BEFHILNOQTWXYZ"); // All letters should return false if we are composing text. override_rewire(hotkey, "processing_text", () => true); diff --git a/frontend_tests/node_tests/message_flags.js b/frontend_tests/node_tests/message_flags.js index 7ffd1fcf06..dccddacc3d 100644 --- a/frontend_tests/node_tests/message_flags.js +++ b/frontend_tests/node_tests/message_flags.js @@ -318,3 +318,24 @@ run_test("collapse_and_uncollapse", ({override}) => { }, }); }); + +run_test("mark_as_unread", ({override}) => { + // Way to capture posted info in every request + let channel_post_opts; + override(channel, "post", (opts) => { + channel_post_opts = opts; + }); + + const msg = {id: 5}; + + message_flags.mark_as_unread([msg.id]); + + assert.deepEqual(channel_post_opts, { + url: "/json/messages/flags", + data: { + messages: "[5]", + op: "remove", + flag: "read", + }, + }); +}); diff --git a/frontend_tests/node_tests/popovers.js b/frontend_tests/node_tests/popovers.js index 27a72f55b7..0be010b470 100644 --- a/frontend_tests/node_tests/popovers.js +++ b/frontend_tests/node_tests/popovers.js @@ -27,6 +27,11 @@ const message_lists = mock_esm("../../static/js/message_lists", { view: { message_containers: {}, }, + data: { + fetch_status: { + has_found_newest: () => true, + }, + }, }, }); mock_esm("../../static/js/message_viewport", { diff --git a/static/js/hotkey.js b/static/js/hotkey.js index 179354cc0e..c605f5d8f0 100644 --- a/static/js/hotkey.js +++ b/static/js/hotkey.js @@ -41,6 +41,7 @@ import * as stream_popover from "./stream_popover"; import * as stream_settings_ui from "./stream_settings_ui"; import * as topic_zoom from "./topic_zoom"; import * as ui from "./ui"; +import * as unread_ops from "./unread_ops"; import {user_settings} from "./user_settings"; function do_narrow_action(action) { @@ -135,6 +136,7 @@ const keypress_mappings = { 80: {name: "narrow_private", message_view_only: true}, // 'P' 82: {name: "respond_to_author", message_view_only: true}, // 'R' 83: {name: "narrow_by_topic", message_view_only: true}, // 'S' + 85: {name: "mark_unread", message_view_only: true}, // 'U' 86: {name: "view_selected_stream", message_view_only: false}, // 'V' 97: {name: "all_messages", message_view_only: true}, // 'a' 99: {name: "compose", message_view_only: true}, // 'c' @@ -940,6 +942,9 @@ export function process_hotkey(e, hotkey) { case "toggle_message_collapse": condense.toggle_collapse(msg); return true; + case "mark_unread": + unread_ops.mark_as_unread_from_here(msg.id); + return true; case "compose_quote_reply": // > : respond to selected message with quote compose_actions.quote_and_reply({trigger: "hotkey"}); return true; diff --git a/static/js/message_flags.js b/static/js/message_flags.js index 85b8a3e358..7b2443eee7 100644 --- a/static/js/message_flags.js +++ b/static/js/message_flags.js @@ -66,6 +66,10 @@ export const send_read = (function () { return add; })(); +export function mark_as_unread(message_ids) { + send_flag_update_for_messages(message_ids, "read", "remove"); +} + export function save_collapsed(message) { send_flag_update_for_messages([message.id], "collapsed", "add"); } diff --git a/static/js/message_list.js b/static/js/message_list.js index e5bb988b57..dfe2a11c3d 100644 --- a/static/js/message_list.js +++ b/static/js/message_list.js @@ -34,10 +34,19 @@ export class MessageList { this.table_name = table_name; this.narrowed = this.table_name === "zfilt"; this.num_appends = 0; + this.reading_prevented = false; return this; } + prevent_reading() { + this.reading_prevented = true; + } + + resume_reading() { + this.reading_prevented = false; + } + add_messages(messages, opts) { // This adds all messages to our data, but only returns // the currently viewable ones. @@ -100,6 +109,10 @@ export class MessageList { return this.data.last(); } + ids_greater_or_equal_than(id) { + return this.data.ids_greater_or_equal_than(id); + } + prev() { return this.data.prev(); } @@ -121,7 +134,14 @@ export class MessageList { } can_mark_messages_read() { - return this.data.can_mark_messages_read(); + /* Automatically marking messages as read can be disabled for + two different reasons: + * The view is structurally a search view, encoded in the + properties of the message_list_data object. + * The user recently marked messages in the view as unread, and + we don't want to lose that state. + */ + return this.data.can_mark_messages_read() && !this.reading_prevented; } clear({clear_selected_id = true} = {}) { diff --git a/static/js/message_list_data.js b/static/js/message_list_data.js index 5410bbab32..a8ad144927 100644 --- a/static/js/message_list_data.js +++ b/static/js/message_list_data.js @@ -42,6 +42,21 @@ export class MessageListData { return this._items.at(-1); } + ids_greater_or_equal_than(my_id) { + const result = []; + + for (let i = this._items.length - 1; i >= 0; i -= 1) { + const message_id = this._items[i].id; + if (message_id >= my_id) { + result.push(message_id); + } else { + continue; + } + } + + return result; + } + select_idx() { if (this._selected_id === -1) { return undefined; diff --git a/static/js/popovers.js b/static/js/popovers.js index 119deabd8d..399b575132 100644 --- a/static/js/popovers.js +++ b/static/js/popovers.js @@ -49,6 +49,7 @@ import * as settings_data from "./settings_data"; import * as settings_users from "./settings_users"; import * as stream_popover from "./stream_popover"; import * as ui_report from "./ui_report"; +import * as unread_ops from "./unread_ops"; import * as user_groups from "./user_groups"; import * as user_profile from "./user_profile"; import {user_settings} from "./user_settings"; @@ -479,6 +480,17 @@ export function toggle_actions_popover(element, id) { editability_menu_item = $t({defaultMessage: "View source"}); } + // Theoretically, it could be useful to offer this even for a + // message that is already unread, so you can mark those below + // it as unread; but that's an unlikely situation, and showing + // it can be a confusing source of clutter. + // + // To work around #22893, we also only offer the option if the + // fetch_status data structure means we'll be able to mark + // everything below the current message as read correctly. + const should_display_mark_as_unread = + message_lists.current.data.fetch_status.has_found_newest() && !message.unread; + const should_display_edit_history_option = message.edit_history && message.edit_history.some( @@ -521,6 +533,7 @@ export function toggle_actions_popover(element, id) { stream_id: message.stream_id, use_edit_icon, editability_menu_item, + should_display_mark_as_unread, should_display_collapse, should_display_uncollapse, should_display_add_reaction_option: message.sent_by_me, @@ -1101,6 +1114,16 @@ export function register_click_handlers() { current_user_sidebar_popover = $target.data("popover"); }); + $("body").on("click", ".mark_as_unread", (e) => { + hide_actions_popover(); + const message_id = $(e.currentTarget).data("message-id"); + + unread_ops.mark_as_unread_from_here(message_id); + + e.stopPropagation(); + e.preventDefault(); + }); + $("body").on("click", ".respond_button", (e) => { // Arguably, we should fetch the message ID to respond to from // e.target, but that should always be the current selected diff --git a/static/js/unread_ops.js b/static/js/unread_ops.js index f2551fab3e..f3a2312b41 100644 --- a/static/js/unread_ops.js +++ b/static/js/unread_ops.js @@ -40,6 +40,23 @@ function process_newly_read_message(message, options) { recent_topics_ui.update_topic_unread_count(message); } +export function mark_as_unread_from_here(message_id) { + /* TODO: This algorithm is not correct if we don't have full data for + the current narrow loaded from the server. + + Currently, we turn off the feature when fetch_status suggests + we are in that that situation, but we plan to replace this + logic with asking the server to do the marking as unread. + */ + const message_ids = message_lists.current.ids_greater_or_equal_than(message_id); + message_lists.current.prevent_reading(); + message_flags.mark_as_unread(message_ids); +} + +export function resume_reading() { + message_lists.current.resume_reading(); +} + export function process_read_messages_event(message_ids) { /* This code has a lot in common with notify_server_messages_read, @@ -81,6 +98,10 @@ export function process_unread_messages_event({message_ids, message_details}) { return; } + if (message_lists.current === message_list.narrowed) { + unread.set_messages_read_in_narrow(false); + } + for (const message_id of message_ids) { const message = message_store.get(message_id); diff --git a/static/js/unread_ui.js b/static/js/unread_ui.js index 4e3b563047..3cbdeaf50c 100644 --- a/static/js/unread_ui.js +++ b/static/js/unread_ui.js @@ -125,6 +125,8 @@ export function initialize() { .all_messages() .filter((message) => unread.message_unread(message)); notify_server_messages_read(unread_messages); + // New messages received may be marked as read based on narrow type. + message_lists.current.resume_reading(); hide_mark_as_read_turned_off_banner(); }); diff --git a/static/styles/dark_theme.css b/static/styles/dark_theme.css index d9ec73c860..376c475fb1 100644 --- a/static/styles/dark_theme.css +++ b/static/styles/dark_theme.css @@ -292,7 +292,10 @@ body.dark-theme { color: hsl(236, 33%, 90%); } - .unread_count { + .unread_count, + /* The actions_popover unread_count object has its own variable CSS, + and thus needs to be repeated here with all three classes for precedence) */ + .actions_popover .mark_as_unread .unread_count { background-color: hsla(105, 2%, 50%, 0.5); } diff --git a/static/styles/popovers.css b/static/styles/popovers.css index 62e336ca02..c2988b5287 100644 --- a/static/styles/popovers.css +++ b/static/styles/popovers.css @@ -145,6 +145,28 @@ ul { } } +.actions_popover { + .mark_as_unread { + .unread_count { + /* The icon for this menu item is in the form of an unread count from + the left sidebar. We reuse much of the shared styling, + but need to override some of the defaults set in app_components.css. */ + display: inline; + float: unset; + line-height: 14px; + font-size: 11px; + font-weight: 550; + margin-right: 2px; + background-color: hsl(200, 100%, 40%); + /* Override random undesired bootstrap style */ + text-shadow: none; + /* Not center aligned but looks better. */ + position: relative; + top: -1px; + } + } +} + .user_popover { width: 240px; diff --git a/static/templates/actions_popover_content.hbs b/static/templates/actions_popover_content.hbs index 0e46b1b04c..f6a8b8af26 100644 --- a/static/templates/actions_popover_content.hbs +++ b/static/templates/actions_popover_content.hbs @@ -27,6 +27,16 @@ {{/if}} + {{#if should_display_mark_as_unread}} +
  • + + 1 + {{t "Mark as unread from here" }} + (U) + +
    • + {{/if}} + {{#if should_display_reminder_option}}
  • diff --git a/static/templates/keyboard_shortcuts.hbs b/static/templates/keyboard_shortcuts.hbs index 619d979e0e..251ff67ee8 100644 --- a/static/templates/keyboard_shortcuts.hbs +++ b/static/templates/keyboard_shortcuts.hbs @@ -234,6 +234,10 @@ + + + {{t 'Mark as unread from selected message' }} + Shift + U + {{t 'Collapse/show selected message' }} - diff --git a/templates/zerver/help/keyboard-shortcuts.md b/templates/zerver/help/keyboard-shortcuts.md index fc9778137e..46b77f7b18 100644 --- a/templates/zerver/help/keyboard-shortcuts.md +++ b/templates/zerver/help/keyboard-shortcuts.md @@ -151,6 +151,8 @@ below, and add more to your repertoire as needed. src="/static/generated/emoji/images/emoji/unicode/1f44d.png" title="thumbs up"/>**: + +* **Mark as unread from selected message**: Shift + U + * **Collapse/show message**: - * **Toggle topic mute**: Shift + M — Muted topics