From 22c1a66da8b96aa1b2484cdca9c91f5e68451c4e Mon Sep 17 00:00:00 2001 From: Kevin Mehall Date: Tue, 2 Jul 2013 14:33:00 -0400 Subject: [PATCH] Don't mark messages as read by visibility. Trac #1428 (imported from commit b67e52d7434220e397ca20ffa49915de6633519c) --- tools/jslint/check-all.js | 2 +- zephyr/static/js/notifications.js | 32 ++++---- zephyr/static/js/stream_list.js | 1 - zephyr/static/js/ui.js | 9 --- zephyr/static/js/zephyr.js | 96 ++++++------------------ zephyr/tests/frontend/tests/06-unread.js | 6 +- 6 files changed, 45 insertions(+), 101 deletions(-) diff --git a/tools/jslint/check-all.js b/tools/jslint/check-all.js index 61b49204fa..59781d1456 100644 --- a/tools/jslint/check-all.js +++ b/tools/jslint/check-all.js @@ -44,7 +44,7 @@ var globals = + ' viewport restart_get_updates force_get_updates' + ' load_more_messages reset_load_more_status have_scrolled_away_from_top' + ' maybe_scroll_to_selected recenter_pointer_on_display suppress_scroll_pointer_update' - + ' process_visible_unread_messages message_range message_in_table process_loaded_for_unread' + + ' message_range message_in_table process_loaded_for_unread' + ' mark_all_as_read message_unread process_read_messages unread_in_current_view' + ' fast_forward_pointer recent_subjects unread_subjects' + ' add_message_metadata' diff --git a/zephyr/static/js/notifications.js b/zephyr/static/js/notifications.js index 518c87ee5b..2d91c4f073 100644 --- a/zephyr/static/js/notifications.js +++ b/zephyr/static/js/notifications.js @@ -30,10 +30,6 @@ exports.initialize = function () { $.each(notice_memory, function (index, notice_mem_entry) { notice_mem_entry.obj.cancel(); }); - - // Update many places on the DOM to reflect unread - // counts. - process_visible_unread_messages(); }).blur(function () { window_has_focus = false; }); @@ -268,18 +264,28 @@ function message_is_notifiable(message) { subs.receives_notifications(message.stream)))); } +function message_is_visible (vp, message) { + if (! notifications.window_has_focus()) { + return false; + } + + var top = vp.visible_top; + var height = vp.visible_height; + + var row = rows.get(message.id, current_msg_list.table_name); + var row_offset = row.offset(); + var row_height = row.height(); + // Very tall messages are visible once we've gotten past them + return (row_height > height && row_offset.top > top) || within_viewport(row_offset, row_height); +} + exports.received_messages = function (messages) { - var i; + var vp = viewport.message_viewport_info(); $.each(messages, function (index, message) { - // We send notifications for messages which the user has - // configured as notifiable, as long as they haven't been - // marked as read by process_visible_unread_messages - // (which occurs if the message arrived onscreen while the - // window had focus). - if (!(message_is_notifiable(message) && unread.message_unread(message))) { - return; - } + if (!message_is_notifiable(message)) return; + if (message_is_visible(vp, message)) return; + if (page_params.desktop_notifications_enabled && browser_desktop_notifications_on()) { process_notification({message: message, webkit_notify: true}); diff --git a/zephyr/static/js/stream_list.js b/zephyr/static/js/stream_list.js index 09d43d9ff2..89089ab3a7 100644 --- a/zephyr/static/js/stream_list.js +++ b/zephyr/static/js/stream_list.js @@ -290,7 +290,6 @@ $(function () { } rebuild_recent_subjects(op_stream[0], subject); } - process_visible_unread_messages(); }); $(document).on('narrow_deactivated.zephyr', function (event) { diff --git a/zephyr/static/js/ui.js b/zephyr/static/js/ui.js index 0d1937d43d..c118daad5c 100644 --- a/zephyr/static/js/ui.js +++ b/zephyr/static/js/ui.js @@ -822,11 +822,6 @@ $(function () { } else if (!have_scrolled_away_from_top) { have_scrolled_away_from_top = true; } - // When the window scrolls, it may cause some messages to - // enter the screen and become read. Calling - // process_visible_unread_messages will update necessary - // data structures and DOM elements. - setTimeout(process_visible_unread_messages, 0); } } @@ -836,10 +831,6 @@ $(function () { scroll_timer = setTimeout(scroll_finished, 100); } - $(window).scroll(function () { - process_visible_unread_messages(); - }); - $(window).scroll($.throttle(50, function (e) { scroll_finish(); })); diff --git a/zephyr/static/js/zephyr.js b/zephyr/static/js/zephyr.js index 3be323f916..741425dbfa 100644 --- a/zephyr/static/js/zephyr.js +++ b/zephyr/static/js/zephyr.js @@ -345,68 +345,6 @@ function process_read_messages(messages) { update_unread_counts(); } -// If we ever materially change the algorithm for this function, we -// may need to update notifications.received_messages as well. -function process_visible_unread_messages(update_cursor) { - // For any messages visible on the screen, make sure they have been marked - // as unread. - if (! notifications.window_has_focus()) { - return; - } - - var selected = current_msg_list.selected_message(); - var vp = viewport.message_viewport_info(); - var top = vp.visible_top; - var height = vp.visible_height; - - // Being simplistic about this, the smallest message is 30 px high. - var selected_row = rows.get(current_msg_list.selected_id(), current_msg_list.table_name); - var num_neighbors = Math.floor(height / 30); - var candidates = $.merge(selected_row.prevAll("tr.message_row[zid]:lt(" + num_neighbors + ")"), - selected_row.nextAll("tr.message_row[zid]:lt(" + num_neighbors + ")")); - - var visible_messages = candidates.filter(function (idx, message) { - var row = $(message); - var row_offset = row.offset(); - var row_height = row.height(); - // Mark very tall messages as read once we've gotten past them - return (row_height > height && row_offset.top > top) || within_viewport(row_offset, row_height); - }); - - if (update_cursor) { - //save the state of respond_to_cursor, and reapply it every time we move the cursor - var probably_from_sent_message = respond_to_cursor; - $.map(visible_messages, function(msg) { - if ((current_msg_list.get(rows.id($(msg))).sent_by_me) && - (current_msg_list.selected_message().id < rows.id($(msg)))) { - // every time we move the cursor, we set respond_to_cursor to false. This should only - // happen if the user initiated the cursor move, not us, so we reset it when processing - // these messages - current_msg_list.select_id(rows.id($(msg)), {then_scroll: true, - from_scroll: true}); - respond_to_cursor = probably_from_sent_message; - } - }); - } - - var mark_as_read = $.map(visible_messages, function(msg) { - var message = current_msg_list.get(rows.id($(msg))); - if (! unread.message_unread(message)) { - return undefined; - } else { - return message; - } - }); - - if (unread.message_unread(selected)) { - mark_as_read.push(selected); - } - - if (mark_as_read.length > 0) { - process_read_messages(mark_as_read); - } -} - function mark_read_between(msg_list, start_id, end_id) { var mark_as_read = []; $.each(message_range(msg_list, start_id, end_id), @@ -750,7 +688,6 @@ function maybe_add_narrowed_messages(messages, msg_list) { new_messages = $.map(new_messages, add_message_metadata); add_messages(new_messages, msg_list); - process_visible_unread_messages(); compose.update_faded_messages(); }, error: function (xhr) { @@ -904,19 +841,32 @@ function get_updates_success(data) { } } - // notifications.received_messages uses values set by - // process_visible_unread_messages and thus must - // be called after it - var i; - var update_cursor = false; - // check if we need to update the cursor, and do so if needed. - for (i = 0; i < messages.length; i++) { - if (messages[i].sent_by_me && narrow.narrowed_by_reply()) { - update_cursor = true; + if (narrow.narrowed_by_reply()) { + // If you send a message when narrowed to a recipient, move the + // pointer to it. + + // Save the state of respond_to_cursor, and reapply it every time + // we move the cursor. + var saved_respond_to_cursor = respond_to_cursor; + + var i; + var selected_id = current_msg_list.selected_message().id; + + // Iterate backwards to find the last message sent_by_me, stopping at + // the pointer position. + for (i = messages.length-1; i>=0; i--){ + if (messages[i].id <= selected_id) break; + if (messages[i].sent_by_me) { + // If this is a reply we just sent, advance the pointer to it. + current_msg_list.select_id(messages[i].id, {then_scroll: true, + from_scroll: true}); + break; + } } + + respond_to_cursor = saved_respond_to_cursor; } - process_visible_unread_messages(update_cursor); notifications.received_messages(messages); compose.update_faded_messages(); stream_list.update_streams_sidebar(); diff --git a/zephyr/tests/frontend/tests/06-unread.js b/zephyr/tests/frontend/tests/06-unread.js index aded7a9654..03740cc82a 100644 --- a/zephyr/tests/frontend/tests/06-unread.js +++ b/zephyr/tests/frontend/tests/06-unread.js @@ -4,8 +4,7 @@ var common = require('../common.js').common; // other tests send a message before this one is run, // changing the unread counts. uncomment line 43 to run // standalone -// Silence jslint errors about the "process_visible_unread_messages" global -/*global process_visible_unread_messages: true */ +// Silence jslint errors about the global /*global keep_pointer_in_view: true */ function send_with_content(content) { @@ -79,8 +78,7 @@ casper.then(function () { // Changing the scroll position in phantomjs doesn't seem to trigger on-scroll // handlers, so unread messages are not handled casper.page.scrollPosition = {top: ypos, left: 0}; - casper.evaluate(function () { keep_pointer_in_view(); - process_visible_unread_messages(); }); + casper.evaluate(function () { keep_pointer_in_view(); }); } var i = 0; scroll_to(0);