Account for unread messages for jtb button visibility #142

smarizvi110 · 2024-09-08T16:13:43Z

This should fix #131. This needs some more testing though I think and is a WIP. Also, I noticed that there was some redundancy in code regarding updating the button's visibility, in that since it isn't encased as a separate function, it's not quite clean code. I can clean this up though, just wasn't sure if I should considering how it was already written. Also, the new UI for Robrix is great but I realized the button should probably be moved a little more to the left as it's too close to the window border on the right. I haven't done that yet but can if it seems okay to do. And finally, would it not be a better idea to set it's colour hex code as some sort of constant rather than using it as a literal?

src/home/room_screen.rs

kevinaboos · 2024-09-10T19:48:23Z

src/home/room_screen.rs

+                        TimelineUpdate::NewItems {items, ..} => {
+                            let portal_list = self.portal_list(id!(timeline.list));
+                            if !portal_list.is_at_end() {
+                                self.unread_message_count += items.len();


this logic doesn't quite seem correct; unfortunately, Matrix makes it quite difficult to correctly determine how many messages are unread.

I think you would want to determine the number of new items that were added beyond the last-known read marker of the current user. Doing this requires tracking the read marker (See #86), which we haven't yet finished.

So for now, you can just show a visual indicator of new messages existing at all, rather than attempting to count how many are considered "unread".

Alright, sounds good!

@kevinaboos @smarizvi110 @ZhangHanDong, The "unread" number got nothing to do with #86, it is based on backend data: unread_notifications' [unreadNotificationsCount](https://matrix-org.github.io/matrix-rust-sdk/matrix_sdk/sync/struct.UnreadNotificationsCount.html)
Without #86, unread notification count should be always increasing and not be able to reset to 0.

Let me look into this.

kevinaboos · 2024-09-10T19:50:18Z

Thanks, I wasn't sure if you were ready for me to review this but I left a few comments anyway.

Regarding clean encapsulation of the JTB button handling code, yes, of course you can move it into a separate function -- no problem there.

smarizvi110 · 2024-09-15T16:59:10Z

Thanks, I wasn't sure if you were ready for me to review this but I left a few comments anyway.

Regarding clean encapsulation of the JTB button handling code, yes, of course you can move it into a separate function -- no problem there.

Whew, thank you for the review actually! I'll try to figure this thing out. Apologies for the delays, just a bit difficult to work on this amongst other things 😅Also on #86, I'll take a look there too. If that could be resolved, then this would be much easier

Co-authored-by: Kevin Boos <[email protected]>

smarizvi110 · 2024-09-15T19:53:46Z

So a few things
The logic I'm thinking of running with is that whenever there's a new message and the user isn't at the bottom, an indicator should be shown. When jtb is pressed, the indicator should not be shown. As suggested, the bool governing this is in TimelineUiState.
Now this should also be the case when the user manually scrolls to the bottom. I just first want to confirm if I'm missing something or not considering what we can work with right now.
Secondly, I had a weird error that I'm not sure about the origins of. This wasn't an issue on the main branch. Specifically
https://github.com/smarizvi110/robrix/blob/jtb-visibility-on-timeline-update/src/home/room_screen.rs#L2683
throws this error at me, even though I don't think it should?

error[E0308]: mismatched types
    --> src\home\room_screen.rs:2683:13
     |
2683 |         let Some(mut inner) = self.borrow_mut() else {
     |             ^^^^^^^^^^^^^^^   ----------------- this expression has type `&mut MessageRef`
     |             |
     |             expected `MessageRef`, found `Option<_>`
     |
     = note: expected struct `MessageRef`
                  found enum `std::option::Option<_>`

kevinaboos · 2024-09-17T14:55:04Z

Whew, thank you for the review actually! I'll try to figure this thing out. Apologies for the delays, just a bit difficult to work on this amongst other things

Of course, and no rush at all! Anything you're willing to contribute is more than welcome!

Also on #86, I'll take a look there too. If that could be resolved, then this would be much easier

Agreed. FYI, another contributor has already "claimed" that issue and is working on it currently.

kevinaboos · 2024-09-17T14:58:58Z

The logic I'm thinking of running with is that whenever there's a new message and the user isn't at the bottom, an indicator should be shown. When jtb is pressed, the indicator should not be shown. As suggested, the bool governing this is in TimelineUiState.
Now this should also be the case when the user manually scrolls to the bottom. I just first want to confirm if I'm missing something or not considering what we can work with right now.

Yep! That's all perfectly accurate.

Secondly, I had a weird error that I'm not sure about the origins of. This wasn't an issue on the main branch. Specifically
https://github.com/smarizvi110/robrix/blob/jtb-visibility-on-timeline-update/src/home/room_screen.rs#L2683
throws this error at me, even though I don't think it should?

Yeah that's not right, that shouldn't cause an error. Not sure what's going on, perhaps there's another error elsewhere that's causing that false error. Are you still getting that error? If so, I can try to run your branch to attempt to reproduce it.

smarizvi110 · 2024-09-25T19:28:45Z

Yep! That's all perfectly accurate.

Great then! I spent some time trying to refine the implementation of the logic but honestly what my branch currently has on github is as good as I could make it for now

Yeah that's not right, that shouldn't cause an error. Not sure what's going on, perhaps there's another error elsewhere that's causing that false error. Are you still getting that error? If so, I can try to run your branch to attempt to reproduce it.

Thank you I'd really appreciate that!

Apologies for the delays too once more. I was actually trying to clean up the logic I have currently as stated above while taking a look at #86 so that once it is fixed, the logic could be adjusted to allow for a count of unread messages too with ease but couldn't make much progress there. And yes, the error I mentioned prior with MessageRef I couldn't understand either. I was going through Rust docs and still couldn't understand 😅

kevinaboos · 2024-10-01T06:07:54Z

sorry, so should I go ahead and review/merge this in, or am I waiting for more changes from you or from a different PR first?

smarizvi110 · 2024-10-01T17:10:51Z

sorry, so should I go ahead and review/merge this in, or am I waiting for more changes from you or from a different PR first?

I think this should be okay, as long as you think it works fine! As for the other PR, I think the code can be adjusted later on when that issue is solved for explicitly showing the number of messages that are unread! For now, I think this should show the presence of unread messages in general.

smarizvi110 · 2024-10-06T15:28:05Z

Okay #86 is apparently done, and an adjustment on it is ready to be approved by the author of the same PR. As soon as that's done, this here can be adjusted too to account for individual number of messages that are still unread

alanpoon · 2024-10-07T13:49:08Z

Hi @smarizvi110 , Could you merge main into your branch and resolve merge conflicts?

smarizvi110 · 2024-10-07T13:54:34Z

Hi @smarizvi110 , Could you merge main into your branch and resolve merge conflicts?

Hey @alanpoon! Would it not be better for #172 to be merged first? The logic here is most likely going to need to be rewritten to adhere to your improved implementation of detecting when messages are read. Let me know, and I'll merge main if you think otherwise. Thank you!

alanpoon · 2024-10-07T16:42:36Z

Hi @smarizvi110 , Could you merge main into your branch and resolve merge conflicts?

Hey @alanpoon! Would it not be better for #172 to be merged first? The logic here is most likely going to need to be rewritten to adhere to your improved implementation of detecting when messages are read. Let me know, and I'll merge main if you think otherwise. Thank you!

Please merge. It has nothing to do with #172.

kevinaboos · 2024-10-07T17:55:00Z

thanks for the review comments everyone! Sorry for the delay, I've been working hard on preparing Robrix for a conference talk in 10 days so I'm behind on reviews.

kevinaboos · 2024-10-07T18:05:17Z

@smarizvi110 can you resolve the conflicts? ping me for a review afterwards. thanks!

…eline-update

smarizvi110 · 2024-10-07T18:19:23Z

thanks for the review comments everyone! Sorry for the delay, I've been working hard on preparing Robrix for a conference talk in 10 days so I'm behind on reviews.

That's so cool! I hope it goes well!

@smarizvi110 can you resolve the conflicts? ping me for a review afterwards. thanks!

@kevinaboos right so the conflicts were resolved but I think the logic can be updated to show the number of unread messages based on what the Alan said in the review conversation above. I'm not sure if I properly understand it, so if anyone has anything further to add on that it'd be great

smarizvi110 · 2024-10-07T19:51:38Z

@kevinaboos based on the code review conversation above, I tried to work on implementing the logic for jtb visibility and unread badge visibility using the matrix SDK unread notification count, though I have not been able to test it properly primarily due to running into the same error as I mentioned above in the comment. If someone could help with this error and verifying if this is at least looks fine, it would be greatly appreciated

alanpoon · 2024-10-08T02:00:04Z

@kevinaboos based on the code review conversation above, I tried to work on implementing the logic for jtb visibility and unread badge visibility using the matrix SDK unread notification count, though I have not been able to test it properly primarily due to running into the same error as I mentioned above in the comment. If someone could help with this error and verifying if this is at least looks fine, it would be greatly appreciated

I saw this compile error in your branch before merging the main. Now it is no longer there. But there are still several other compilation errors. So far, I have not seen any of your commits that does not have compile error. It might be faster for me to create a new pull request than to resolve your compilation errors.

alanpoon · 2024-10-08T02:43:07Z

So a few things The logic I'm thinking of running with is that whenever there's a new message and the user isn't at the bottom, an indicator should be shown. When jtb is pressed, the indicator should not be shown. As suggested, the bool governing this is in TimelineUiState. Now this should also be the case when the user manually scrolls to the bottom. I just first want to confirm if I'm missing something or not considering what we can work with right now. Secondly, I had a weird error that I'm not sure about the origins of. This wasn't an issue on the main branch. Specifically https://github.com/smarizvi110/robrix/blob/jtb-visibility-on-timeline-update/src/home/room_screen.rs#L2683 throws this error at me, even though I don't think it should?
error[E0308]: mismatched types
    --> src\home\room_screen.rs:2683:13
     |
2683 |         let Some(mut inner) = self.borrow_mut() else {
     |             ^^^^^^^^^^^^^^^   ----------------- this expression has type `&mut MessageRef`
     |             |
     |             expected `MessageRef`, found `Option<_>`
     |
     = note: expected struct `MessageRef`
                  found enum `std::option::Option<_>`

Your MessageRef's set_data is missing "cx: &mut Cx" as second function parameter. MessageRef is tied to procedure macros generated in Message Struct.

impl MessageRef {
    pub fn set_data(&self, cx: &mut Cx, can_be_replied_to: bool, item_id: usize) {
        let Some(mut inner) = self.borrow_mut() else {
            return;
        };
        inner.set_data(can_be_replied_to, item_id);
    }
}

alanpoon · 2024-10-10T07:40:45Z

src/home/room_screen.rs

+            tokio::spawn(async move {
+                if let Err(e) = room_screen.update_unread_notifications().await {
+                    log!("Failed to update unread notifications: {:?}", e);
+                }
+            });


You should be updating the Client state inside process_timeline_updates function instead of using tokio spawn when processing backend data.

How do you recommend going about that considering update_unread_notifications() is async?

alanpoon · 2024-10-10T07:42:32Z

src/home/room_screen.rs

@@ -940,9 +974,11 @@ struct RoomScreen {
    #[rust] tl_state: Option<TimelineUiState>,
    /// 5 secs timer when scroll ends
    #[rust] fully_read_timer: Timer,
+    /// The Matrix SDK client (optional until initialized).
+    #[rust] client: Option<Client>,


shouldn't put client here in the RoomScreen Struct

Do you recommend letting this remain as global in the file, or do you have a better suggestion? The rationale for putting it here was because it's only used for the room in question that the user is using, and not elsewhere in this file.

I have created a new combine PR with the #86 issue here: #206 . Maybe you can see how I did it. You should be using sliding_async.rs for making matrix request

@alanpoon Thank you so much! I'll take this opportunity to learn more.

alanpoon · 2024-10-10T07:48:26Z

src/home/room_screen.rs

+        }
+    }
+
+    fn handle_jump_to_bottom_visibility(&mut self, cx: &mut Cx, actions: &ActionsBuf) {


There is no need to wrap an existing piece of code in a function that is used only once. This unnecessary code change makes it hard for other to review your code change.

kevinaboos · 2024-10-29T18:19:05Z

Hi @smarizvi110, apologies for the lengthy delay in my review, was attending a conference and then had some travel, and got sick twice. 😑

I think the scope of this PR has grown far too much. Let's limit it to just one small targeted feature: re-showing the JTB button when new messages occur. That's it. Let's not try to overcomplicate it with calculating the number of unread messages.

If you can reduce it back down to just those changes (should only be like a dozen lines of code or so), then I'm happy to merge.

smarizvi110 · 2024-10-30T20:03:52Z

Hi @smarizvi110, apologies for the lengthy delay in my review, was attending a conference and then had some travel, and got sick twice. 😑

I think the scope of this PR has grown far too much. Let's limit it to just one small targeted feature: re-showing the JTB button when new messages occur. That's it. Let's not try to overcomplicate it with calculating the number of unread messages.

If you can reduce it back down to just those changes (should only be like a dozen lines of code or so), then I'm happy to merge.

No worries at all @kevinaboos! It's so nice to hear from you! I hope your conference went well; I believe you mentioned it prior too. Sorry to hear you fell ill so severely. It really seems like that is inevitable at this time of the year. I hope you managed to recover fully!

As for this PR, I believe #206 does a lot of work regarding #86 which does try to account for the number of unread messages. I took a look at #206 again and noticed it needs a little more work based on your code review there. If you nonetheless think that this PR should go through with some edits, I'll try to do that. I'll have to look into [the Matrix SDK Unread Notifications Count]((https://matrix-org.github.io/matrix-rust-sdk/matrix_sdk/sync/struct.UnreadNotificationsCount.html) again though because I'm afraid I don't quite understand it based on the code reviews conducted by the author of #206, as well as their disapproving comments here and the contents of #206. Could I get some insight from you on this, if possible?

kevinaboos · 2024-10-30T22:26:56Z

My previous comment was basically saying that this PR should completely skip trying to deal with the unread message count (which I don't think the other PR has perfectly nailed yet either), and then we can merge it such that your work here doesn't go to waste.

So, the re-summarize, this PR should just do one very simple thing: determine whether we should show the jump to bottom button based on two actions/events:

(already done) the user scrolled up such that the bottom is no longer in the viewport.
A new message was received in the timeline, but the user wouldn't know about it unless they manually scrolled down.

Recall that the entire point of this PR (and of issue #131) was only number 2 above. No need to complicate it with extra things.

If you are confused or don't wish to make the changes, that's fine, I am happy to do so. Just let me know, as I wanted to let you retain credit for your contributions-in-progress here.

kevinaboos · 2024-11-09T05:35:59Z

In order to sort things out a bit (as this PR and #206 are in a bit of a messy state), I have implemented this directly in #247, which required some fixes to makepad.

If you're interested in the sort of code I was looking for, check out #247.

@smarizvi110 thanks for your contributions here! I credited you with some contributions as a co-author in the merge commit of #247. Thanks!

smarizvi110 · 2024-11-10T11:41:57Z

In order to sort things out a bit (as this PR and #206 are in a bit of a messy state), I have implemented this directly in #247, which required some fixes to makepad.

If you're interested in the sort of code I was looking for, check out #247.

@smarizvi110 thanks for your contributions here! I credited you with some contributions as a co-author in the merge commit of #247. Thanks!

Thank you @kevinaboos! Yes I'd love to go over the changes you made to learn more about the codebase. Really sorry for being unresponsive, just swamped with coursework and graduate apps these days 😅

kevinaboos · 2024-11-12T03:57:15Z

No problem at all, don't worry about it! I just wanted to go ahead and close this issue since some other folks were also debating about how to deal with things like this.

Attempt to account for unread messages

d76ffeb

kevinaboos reviewed Sep 10, 2024

View reviewed changes

src/home/room_screen.rs Outdated Show resolved Hide resolved

kevinaboos reviewed Sep 10, 2024

View reviewed changes

src/home/room_screen.rs Outdated Show resolved Hide resolved

kevinaboos reviewed Sep 10, 2024

View reviewed changes

smarizvi110 and others added 4 commits September 15, 2024 22:00

Typo fix

a455cbd

Co-authored-by: Kevin Boos <[email protected]>

Fix logic

98a8527

jtb press updates unread message bool

ff36ecc

Manual scroll to end updates unread messages bool

bd66e57

smarizvi110 changed the title ~~Account for unread messages for jtb button visibility and display their number~~ Account for unread messages for jtb button visibility Sep 29, 2024

Merge remote-tracking branch 'origin/main' into jtb-visibility-on-tim…

7c8990b

…eline-update

Attempt to add unread message badge and jtb logic through Matrix SDK

b8824c6

alanpoon reviewed Oct 10, 2024

View reviewed changes

kevinaboos added the waiting-on-author This issue is waiting on the original author for a response label Nov 7, 2024

kevinaboos mentioned this pull request Nov 9, 2024

Show JTB button with unread message badge upon new out-of-view messages #247

Merged

kevinaboos closed this Nov 9, 2024

Account for unread messages for jtb button visibility #142

Account for unread messages for jtb button visibility #142

Conversation

smarizvi110 commented Sep 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanpoon Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinaboos commented Sep 10, 2024

smarizvi110 commented Sep 15, 2024 • edited Loading

smarizvi110 commented Sep 15, 2024

kevinaboos commented Sep 17, 2024

kevinaboos commented Sep 17, 2024

smarizvi110 commented Sep 25, 2024 • edited Loading

kevinaboos commented Oct 1, 2024

smarizvi110 commented Oct 1, 2024 • edited Loading

smarizvi110 commented Oct 6, 2024

alanpoon commented Oct 7, 2024

smarizvi110 commented Oct 7, 2024 • edited Loading

alanpoon commented Oct 7, 2024

kevinaboos commented Oct 7, 2024

kevinaboos commented Oct 7, 2024

smarizvi110 commented Oct 7, 2024 • edited Loading

smarizvi110 commented Oct 7, 2024

alanpoon commented Oct 8, 2024

alanpoon commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinaboos commented Oct 29, 2024

smarizvi110 commented Oct 30, 2024 • edited Loading

kevinaboos commented Oct 30, 2024

kevinaboos commented Nov 9, 2024

smarizvi110 commented Nov 10, 2024

kevinaboos commented Nov 12, 2024

alanpoon Oct 7, 2024 •

edited

Loading

smarizvi110 commented Sep 15, 2024 •

edited

Loading

smarizvi110 commented Sep 25, 2024 •

edited

Loading

smarizvi110 commented Oct 1, 2024 •

edited

Loading

smarizvi110 commented Oct 7, 2024 •

edited

Loading

smarizvi110 commented Oct 7, 2024 •

edited

Loading

smarizvi110 commented Oct 30, 2024 •

edited

Loading