project-robius
Account for unread messages for jtb button visibility
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?
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.
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
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<_>`
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.
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.
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 😅
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?
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.
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
Hi @smarizvi110 , Could you merge main into your branch and resolve merge conflicts?
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!
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.
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.
@smarizvi110 can you resolve the conflicts? ping me for a review afterwards. thanks!
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
@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
@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.
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);
}
}
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.
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?
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.
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!
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 😅
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.