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

Display day change messages #520

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jspricke
Copy link
Contributor

The WA FAQ proposes to do this via trigger:

https://github.com/ubergeek42/weechat-android/wiki/FAQ#i-dont-see-date-change-messages

But this does not work for other buffers and produces log messages.

@jspricke jspricke force-pushed the add_daychange branch 5 times, most recently from 71f5b94 to 023d655 Compare June 13, 2021 11:13
Copy link
Collaborator

@oakkitten oakkitten left a comment

Choose a reason for hiding this comment

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

i'm not sure if this is an improvement over the faq variant. i like the latter because:

  • day change line appears directly at midnight, and not when the next line appears
  • line text easily customizable by user
  • requires no effort on our part

the downside of this is that consecutive date changes are not squashed but i actually prefer this behavior over WeeChat's. as for

But this does not work for other buffers and produces log messages.

i suppose you could add the tag no_log to make the line skip the logs? not sure in which buffers this doesn't work.

@jspricke
Copy link
Contributor Author

i'm not sure if this is an improvement over the faq variant. i like the latter because:

  • day change line appears directly at midnight, and not when the next line appears

What is the use of that? If there are no lines does it carry any information apart from the time you have already in the corner of your display? ;)

  • requires no effort on our part

This does not really require effort on our part either (apart from pressing merge at some point :P). Contrary the FAQ version requires effort from every user (and they need to find that first).

But this does not work for other buffers and produces log messages.

i suppose you could add the tag no_log to make the line skip the logs? not sure in which buffers this doesn't work.

The most important part here is that I didn't find a way to add the trigger to a lot of buffers (like wee-slack, weechat-matrix...)

@oakkitten
Copy link
Collaborator

well the date change line has very limited utility on the whole. when does it matter that the date changed? i suppose it mostly matters when the date change itself, or the whole day, is important, e.g. it's nye or someone's birthday. sure, the same information is in the corner of your display but it's neat to also see the day change in real time in chat. i guess?

Contrary the FAQ version requires effort from every user (and they need to find that first).

can't argue here :|

didn't find a way to add the trigger to a lot of buffers

yeah both /allpv an /allchan are irc plugin-specific commands; i suppose matrix etc should have similar commands so perhaps we need to update the faq with those

@jspricke
Copy link
Contributor Author

jspricke commented Jun 14, 2021

well the date change line has very limited utility on the whole. when does it matter that the date changed? i suppose it mostly matters when the date change itself, or the whole day, is important, e.g. it's nye or someone's birthday. sure, the same information is in the corner of your display but it's neat to also see the day change in real time in chat. i guess?

It matters when you scroll back in history and want to get en estimate where you are. I after have lines at specific dates (log messages, interaction at around the same time) it is really hard to separate different dates there.

yeah both /allpv an /allchan are irc plugin-specific commands; i suppose matrix etc should have similar commands so perhaps we need to update the faq with those

they don't have such commands. Do you really think it is better to implement this behavior in every script instead of do it similar how Weechat does it?

@oakkitten
Copy link
Collaborator

oakkitten commented Jun 14, 2021

they don't have such commands. Do you really think it is better to implement this behavior in every script instead of do it similar how Weechat does it?

well you have to separate chat buffers from other buffers somehow don't you?

e: also you'd have to retrieve WeeChat's time zone

e2: actually we are ignoring WeeChat's time zone completely atm

This makes sure that lines are only added through the Lines.addLast()
interface.
@jspricke jspricke force-pushed the add_daychange branch 3 times, most recently from 320b049 to 1fe18b9 Compare June 17, 2021 09:42
@jspricke
Copy link
Contributor Author

I've rewritten this based on our discussion. Here is a video of a day change:

out.mp4

@jspricke jspricke force-pushed the add_daychange branch 3 times, most recently from 4032942 to ca0bdd8 Compare June 18, 2021 07:42
@jspricke
Copy link
Contributor Author

jspricke commented Jun 18, 2021

fixed the read marker:

out.mp4

@jspricke jspricke force-pushed the add_daychange branch 3 times, most recently from 03b6fe0 to 2c15748 Compare June 18, 2021 19:40
@oakkitten
Copy link
Collaborator

oakkitten commented Jun 18, 2021

to summarize the current situation:

  • the trigger way:

    • pros:
      • line text easily customizable by user
      • no code to maintan on our part
    • cons
      • consecutive date changes are not squashed¹
      • appears at 00:00 at the time zone of WeeChat, which can differ from that of Android²
      • only works on IRC buffers³
      • user has to discover this somehow
  • new way, at the time being:

    • pros:
      • consecutive date changes are squashed
      • works out of the box
    • cons:
      • doesn't appear at midnight sharp, but only along with the next message
      • is probably broken around SquiggleLine
      • the animation is broken, see above⁴
      • ignores the setting weechat.look.day_change and date change formats
      • works on all buffers, including those where date change is disabled (see date_change in hdata)⁵ — probably should be disabled for free buffers?
      • might impact battery life a bit?
      • increases code complexity a lot

¹ i actually like this
² can be solved by using WeeChat's time zone, but it's more reasonable to use local time zone
³ apparently WeeChat doesn't have an /allbuffers kind of a command
⁴ the animation could be improved to account for two lines appearing at the same time, but it isn't horrible
buffer.date_change seems to be always on for all buffers, including buffers such as /fset, and apparently no script or plugin sets this option, so it might be ignored

@jspricke
Copy link
Contributor Author

The animation was only broken because I hacked the implementation to add (the same) date line every time. Here is the correct animation:

out.mp4

@jspricke
Copy link
Contributor Author

I added the option to disable this with the day_change property in hdata.

@oakkitten oakkitten force-pushed the master branch 2 times, most recently from 3789f56 to 199d6cc Compare June 22, 2021 02:31
@oakkitten
Copy link
Collaborator

oakkitten commented Jun 23, 2021

it's not clear how this should work with squiggle line. squiggle separates continuous messages whenever there's a break due to connectivity loss, as in

message 1
message 2
(squiggle)
message 5
message 6

if messages 2 and 5 have different dates, what about date change line? there seems to be 3 options:

  • don't show it at all
  • put it either before or after the squiggle
  • put it in the “middle” of the squiggle, as in

    message 1
    message 2
    (squiggle)
    (date change line)
    (squiggle)
    message 5
    message 6

while the last option seems to make the most sense to me, it's not clear how this should work with line filtering. note that squiggles can appear either in both filtered/unfiltered modes or only in unfiltered mode

@jspricke
Copy link
Contributor Author

if messages 2 and 5 have different dates, what about date change line? there seems to be 3 options:

  • don't show it at all
  • put it either before or after the squiggle
  • put it in the “middle” of the squiggle, as in

    message 1
    message 2
    (squiggle)
    (date change line)
    (squiggle)
    message 5
    message 6

while the last option seems to make the most sense to me, it's not clear how this should work with line filtering. note that squiggles can appear either in both filtered/unfiltered modes or only in unfiltered mode

I agree with the options and currently the day change line is put in front of the squiggle where the day change happened. So far my tests didn't show any preference for either option, I see squiggle lines mostly for a short time and rather like a general indication that something is missing. Contrary the day change line is more like metadata. I would propose to use the current version for now and change it in case users bring argument for other options.

@jspricke
Copy link
Contributor Author

@oakkitten this should fix your last comments in IRC.

@jspricke jspricke force-pushed the add_daychange branch 3 times, most recently from 35ada9c to a067238 Compare July 11, 2021 21:05
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.

2 participants