Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Order alerts by type and sent time (+polish) #418

Merged
merged 7 commits into from Dec 19, 2015
Merged

Conversation

ArneBab
Copy link
Contributor

@ArneBab ArneBab commented Oct 31, 2015

(just what the title says)

drak@kaverne added 2 commits October 30, 2015 19:10
Remove composed and received time since they aren’t useful to users.
if(isN2NTM0 && isN2NTM1) { // newest first
if(a0.getUpdatedTime() < a1.getUpdatedTime()) return 1;
else if(a0.getUpdatedTime() > a1.getUpdatedTime()) return -1;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ordering by updated time is always more sensible than ordering by hash code. I don't see the need to restrict this to N2NTMUserAlert. Apart from that, I'd prefer the UserAlertManager to be as agnostic as possible of the kind of UserAlert.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I should just leave out the (isN2NTMx)?

@bertm
Copy link
Contributor

bertm commented Oct 31, 2015

Are you aware of the possibilities git-rebase offers to just reword commit messages? (git rebase -i HEAD~2, mark all commits with reword, rephrase commit messages one by one, done)

drak@kaverne added 2 commits November 2, 2015 22:22
This fits the goal of showing the newest message first,
and if a friend was offline for some time,
the sent time would not fit that.

Thanks to bertm.
@ArneBab
Copy link
Contributor Author

ArneBab commented Nov 2, 2015

I’m using Mercurial to interface with github, there the equivalent command is called histedit, but I wasn’t in my source tree when I filed the pull request — and as you saw it took me 3 days to get into it again, so I decided to say sorry instead of delaying the pull request

But still, thank you for the short guide. I might need it one day.

The comment messages are fixed now.

@bertm
Copy link
Contributor

bertm commented Nov 2, 2015

No problem

Looks good to me.

@ArneBab
Copy link
Contributor Author

ArneBab commented Nov 2, 2015

Thank you for reviewing!

@ArneBab ArneBab changed the title Order Node-to-Node messages by sent time+polish Order alerts by type and sent time (+polish) Nov 3, 2015
@@ -1039,7 +1039,7 @@ N2NTMToadlet.tooLarge=The file you have uploaded through your browser is ${attem
N2NTMToadlet.sizeWarning=Please note that browser-based uploading has a limit of ${limit}.
N2NTMToadlet.uploadFailed=Failed to upload file.
N2NTMUserAlert.delete=Delete
N2NTMUserAlert.header=From: ${from} (composed ${composed} | sent ${sent} | received ${received})
N2NTMUserAlert.header=From: ${from} (sent ${sent})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Composed might not be especially useful information, but I think both sent and received are. I know when I want to know about the timing of texts when I come out of a basement without cell coverage or something I'll want to know when it was sent, but if I come back to my phone to something like "ready to go in an hour" I'll want to know when it was received to know if I ever had a chance of responding in time. Thoughts?

I'm tired so this is too rambly but hopefully it makes sense anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies for not catching this earlier, but sentTime can apparently be deceiving: it is set as the time at which the N2N message is dispatched to the Node's MessageCore (source) where it is queued for transmission over the network, if and only if the peer is currently connected. It should be really close to receivedTime if we ignore clock skew problems.

composedTime might be the one you are after knowing.

@ArneBab
Copy link
Contributor Author

ArneBab commented Nov 6, 2015

This should fix both (and the comment is now actually useful, stating that it orders newest first).

I cut out the “sent” from the alert, because I think that it’s not actually a relevant information for users, so it just adds to the line noise.

@Thynix Thynix merged commit ffa0780 into hyphanet:next Dec 19, 2015
@ArneBab
Copy link
Contributor Author

ArneBab commented Dec 25, 2015

thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants