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

Conversation

Projects
None yet
3 participants
@ArneBab
Copy link
Contributor

ArneBab commented Oct 31, 2015

(just what the title says)

drak@kaverne added some commits Oct 30, 2015

drak@kaverne
l10n: Simplify N2NTM useralert header
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;
}

This comment has been minimized.

@bertm

bertm Oct 31, 2015

Member

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.

This comment has been minimized.

@Thynix

Thynix Nov 1, 2015

Member

Agreed on both.

This comment has been minimized.

@ArneBab

ArneBab Nov 2, 2015

Contributor

so I should just leave out the (isN2NTMx)?

@@ -117,6 +117,11 @@ public FCPMessage getFCPMessage() {
composedTime, sentTime, receivedTime, messageText);
}

@Override
public long getUpdatedTime() {
return sentTime;

This comment has been minimized.

@bertm

bertm Oct 31, 2015

Member

It might be slightly more sensible to order by receive time: sent time is set by the sender, so does not necessarily lead to "newest first" behaviour (consider a friend being disconnected for some time).

This comment has been minimized.

@ArneBab

ArneBab Nov 2, 2015

Contributor

that’s a good point… thanks!

@bertm

This comment has been minimized.

Copy link
Member

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 some commits Nov 2, 2015

drak@kaverne
useralerts.N2NTM: order by received time instead of sent time
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 ArneBab force-pushed the ArneBab:next branch from d5e0a8a to 5f4c14b Nov 2, 2015

@ArneBab

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

bertm commented Nov 2, 2015

No problem ☺

Looks good to me.

@ArneBab

This comment has been minimized.

Copy link
Contributor

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})

This comment has been minimized.

@Thynix

Thynix Nov 4, 2015

Member

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.

This comment has been minimized.

@bertm

bertm Nov 4, 2015

Member

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.

@@ -158,6 +158,9 @@ public int compare(UserAlert a0, UserAlert a1) {
int classHash1 = a1.getClass().hashCode();
if(classHash0 > classHash1) return 1;
else if(classHash0 < classHash1) return -1;
// Then go by sent time

This comment has been minimized.

@Thynix

Thynix Nov 4, 2015

Member

This comment says sent time; the getUpdatedTime() implementation says receivedTime.

@ArneBab

This comment has been minimized.

Copy link
Contributor

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 freenet:next Dec 19, 2015

@ArneBab

This comment has been minimized.

Copy link
Contributor

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