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

date change messages are chat_day_change color, multiple messages gerated for diff of 1-5 days #677

Merged
merged 2 commits into from Nov 23, 2015

Conversation

pmelanson
Copy link
Contributor

PR for discussion.

Two changes here,

  1. the date change message is now a pretty color (specifically, chat_day_change). To do this, I had to change this one line in js/weechat.js. I'll point that out in a line comment.
  2. If the difference between the current date and the previous message's date is 1-5 dates, inclusive, then it will generate that many date change messages. If it's outside of that range (for example, negative date ranges) only one date change message is generated. I've attached some screenshots of this behaviour.

http://i.imgur.com/7OGs1jF.png (sorry for imgur, git wasn't uploading it properly. I'm on greyhound bus wifi atm.)

@@ -290,7 +290,7 @@
var ret = {};
var optionCode = parseInt(m[1]);

if (optionCode > 43) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why was this 43? seems an arbitrary number, and was smaller than the size of the color array

Copy link
Member

Choose a reason for hiding this comment

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

@torhve do you know? Or maybe @eepp (who wrote/committed it)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(credit squirrel on IRC) 43 was actually the length, but for some reason using chat_day_change didn't work until I made this change. But then I tested it without this change just now and it works. I dunno what's up. In any case, it works and is a little bit more descriptive.

@pmelanson pmelanson force-pushed the date-change branch 4 times, most recently from 19663da to 30b564a Compare November 21, 2015 03:41
@pmelanson
Copy link
Contributor Author

http://i.imgur.com/wt0x85V.png for demo of new 2date behaviour

@@ -20,12 +20,27 @@ weechat.factory('handlers', ['$rootScope', '$log', 'models', 'plugins', 'notific
};

// inject a fake buffer line for date change
var injectDateChangeMessage = function(message, buffer, date) {
var content = "Date changed to " + date.toDateString();
var injectDateChangeMessage = function(message, buffer, d1, d2) {
Copy link
Member

Choose a reason for hiding this comment

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

d1 and d2 aren't particularly helpful variable names. We try to keep them as meaningful as possible, would you mind changing them to prev_date and curr_date or old_date and new_date or something along those lines? And maybe next_date or day_after_new/day_after_curr for d1_plus_one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for catching this, this is going to be my new year's resolution

@lorenzhs
Copy link
Member

Very cool, I just have some minor odds and ends (see inline comments). Thanks!

@lorenzhs
Copy link
Member

Ah wait the PR is for the date change branch, I'll fix it myself.

lorenzhs added a commit that referenced this pull request Nov 23, 2015
date change messages are chat_day_change color, multiple messages gerated for diff of 1-5 days
@lorenzhs lorenzhs merged commit 38d2f33 into glowing-bear:date-change Nov 23, 2015
@lorenzhs
Copy link
Member

I'm getting a "00:00 <──> Date changed to Sat Jun 27 2015 (1 days from Fri Jun 26 2015)" message in one of my buffers and I can't figure out why. Anyone got any ideas? (the date changed by two days, from Thu 25 to Sat 27)

lorenzhs added a commit that referenced this pull request Nov 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants