external_script: show notification with full output on click, refresh only on button_refresh

[maximbaz]

Replaces #1431.

Show notification with full output on click.
Refresh only on button_refresh click.
Still needs #1432 for most scripts that I use.

[lasers]

Why am I getting fatal: Cannot setup tracking information; starting point 'maximbaz/external_script_notification' is not a branch. from just you and @cyrinux?

[maximbaz]

Don't know, what git commands do you use? Are you sure you fetched my remote before checking out the branch?

[lasers]

[chris:~/src/py3status] master(+168/-1)+ ± git fetch maximbaz
[chris:~/src/py3status] master(+168/-1)+ ± git checkout https://github.com/ultrabug/py3status/pull/1439
fatal: Cannot setup tracking information; starting point 'maximbaz/external_script_notification' is not a branch.

[lasers]

I want to avoid count_ prefix here. Use {line} just like {path}.

If one day, somebody want specific lines, then we can do {line_3} too without messing up the placeholders. Also, it is much shorter and clean.

• [\?if=line&color=line {line} lines]
• [\?if=count_lines&color=count_lines {count_lines} lines].

[maximbaz]

Why? In my mind that's more confusing, why line_1 is a string and line is suddenly a number.

num_lines maybe?

[lasers]

In my mind...

    {total_hours}    total time in hours, eg 0
    {total_minutes}  total time in minutes, eg 24
    {total_seconds}  total time in seconds, eg 1499

vs

    {hours_total}    hour total, eg 0
    {minutes_total}  minute total, eg 24
    {seconds_total}  second total, eg 1499

I like the first one. More organized / future proof. num_lines is same as the second one.

I avoid plurals so relevant placeholders can stay together.

    {battery}        number of batteries <-- i like this
    {batteries}      number of batteries <-- we lost relevance
    {battery_bar}
    {battery_percent}
    {battery_status}
    {battery_time}
    {count_battery}  number of batteries <-- we lost prefix
    {num_battery}    number of batteries <-- we lost prefix

Fake sysdata example -- we retain all prefixes

    {cpu_percent}     cpu used percentage, eg 11.82
    {cpu_temp}        cpu temperature, eg 60.00
    {cpu_unit}        cpu temperature unit, eg C
    {load_1min}       load average during the last 1-minute, eg 1.44
    {load_5min}       load average during the last 5-minutes, eg 1.66
    {load_15min}      load average during the last 15-minutes, eg 1.52
    {max_percent}     maximum used percentage of cpu, mem, swap percentages
    {mem_percent}     memory used percentage, eg 92.95
    {mem_total}       memory total, eg 7.72
    {mem_total_unit}  memory total unit, eg GiB
    {mem_used}        memory used, eg 7.17
    {mem_used_unit}   memory used unit, eg GiB
    {swap_percent}    swap used percentage, eg 25.88
    {swap_total}      swap total memory, eg 7.73
    {swap_total_unit} swap total memory unit, eg GiB
    {swap_used}       swap used, eg 2.00
    {swap_used_unit}  swap used memory unit, eg GiB

[maximbaz]

how about line_count then? no plurals, will stay next to line_? and yet explains well that this is a count.

[lasers]

Similar things with percents... eg {brightness} instead of {brightness_percent}. Sometimes, we add _percent because of sysdata's {mem_used}. {mem_used} is same as {line} since we don't suffix {mem_used} with anything.

We have to consider format too. If we prefix this with _count, then everything gets expanded much longer. Placeholder, if statements, color thresholds, etc. This approach where we don't be explicit with counts seems to work well for several modules I worked on... such as mail where I don't do count_mail. The users can use [\?if=mail {mail} Mail] instead of [\?if=count_mail {count_mail} Mail].

[maximbaz]

To be honest I don't see an issue with verbose configuration, at least it's readable without referring to docs. To a new user, it will not be obvious that format = '{line}' will actually print a number.

But, I am ready to give up on this and change to line if you really want so 🙂

[lasers]

Please change. We need to get away with long names if we can... since most of the modules are small or simple enough not to warrant spelling out _count. Those simple module often are being used with threshold and other things too.

sysdata, pomodoro, taskwarrior, etc are not simple modules, but it looks like we can avoid them too... so I want to take advantage of that situation and extend this to all modules. It's a matter of verbosity level... I chalk it down a little.

[maximbaz]

done 🙂

[lasers]

This hardcodes the notification with whole output. We want self.format_notification for this so users can specify if they want whole output, first line output, or number of lines in an output... just like format. It looks like you should make another placeholder to display whole output.

Based on current code, {output} is technically {line_1} right now because if we try script_path = 'cal', we will get first line only.. instead of whole cal in {output}.

[maximbaz]

I agree, but I don't want to touch formatting in this PR, it will grow too much. Formatting done properly, it should keep output for the entire output, add line_? for per-line parsing, probably do something about color line (currently is hardcoded to be the second line), as well as introduce format_notification. This PR is a minimal change only needed to show full content in a notification and not refresh module on click.

[lasers]

The way I see it... We have a choice. Implement this PR where it will do things wrong and will do things wrong for long time or implement a PR that does things right in first place and not having to worry about it for long time.

For instance, if an user drops in... asking us how to turn off the notification because he simply want to refresh his script without dealing with notifications, can it be turned off right now?

If not, then we have to fix it eventually. That's why I want to fix this right in first place for the notifications. The ideal behavior is to leave notifications off by default too.

[maximbaz]

I don't think not being able to format notification message is necessarily wrong, current behavior shows first line of the output on the status bar, this change allows you to additionally see the rest of the lines on click.

As for disabling notifications, that's why initially I implemented it with two buttons, button_refresh and button_show_notification. Should I return to that approach?

[lasers]

battery_level have notification options, but no button_show_notification. It has this.

    notification: show current battery state as notification on click
        (default False)

Today, self.format_notification = None should replace that option. I do not want to spend time adding then supporting button_show_notification in many modules. We're here now... so we should add self.format_notification now too to avoid unnecessary button_show_notification and hardcoded self.output.

I see I made mistakes in some modules too, so it'll be hard to undo some of them.. but we can avoid the mistake here with notification + button configs.

[lasers]

Ask @cyrinux to help you with format_notification. It won't be a problem... The problem is that {output} placeholder is printing one line... so I assume we have to find a different name that explains {output}, but not {output}... eg {output_full}... but {output} is such a good name to use.

[lasers]

@maximbaz Okay, the solution is make format_notification = None, but we won't document {output_full} placeholder so only you and few others will know about it... The rest can use {output} (aka 1line) or {line} (aka count_lines).

If/when we break this module someday, we could do it at that time.

So you just have to implement format_notification. Pass self.py3.safe_format object to self.py3.notifiy_user() instead of self.output string too (so we don't have to format this twice).

[maximbaz]

Done, now it is possible to set self.format_notification = None to disable notification. I intentionally implemented as least as possible functionality to keep this PR focused on its original goal.

[maximbaz]

It is named output because it is a proper name, and a short one as you seem to prefer - the fact that in another place output currently only prints the first line is the problem of that code.

[lasers]

This config need to be None by default so we don't annoy other users.

[maximbaz]

Why would they be annoyed? Middle click (default button for refresh) still works as it was working before, the rest of the buttons had no action assigned to them, now they do something useful - show full output of the script.

[lasers]

Everybody have different goals with their external_script and we don't know what their goals are. It can be something trivial like to clicking immediately to update a number of mails or updates... and not wanting to see a notification about last number of mails or updates... or to simply execute something periodically with static text... only to be notified about the same good ol' static text.

I assume 99% of time, it will be be same as the one they have on their bar. You want New! full output in your notification. That's fine and we can add it... but not with default notification = '{output}' or notification = '{output_full}'.

With None by default, we can quietly introduce this new feature without literally notifying people about it.

[maximbaz]

Clicking to refresh is what button_refresh is for, it doesn't show notifications and is there exactly for these cases. You said so yourself, it is good to keep button_refresh to 2 by default so that everyone knows: middle click is refresh with no extra actions. Other buttons do extra actions.

I don't see this as a feature "only for me", which I should hide from other users, I genuinely think it makes sense to be on by default. Same in battery_level by the way, I don't see why this is an opt-in feature, in my mind button 1 should always show notification and button 2 should be for silent refreshes.

[lasers]

Missing {line} too. See below.

[lasers]

Leave this alone as-is. Put output inside the dict instead... and assign self.py3.safe_format() to self.composite. Feed composite to self.py3.notify_user.. then we can haz all same placeholders.

Don't document {output_full}. Fewer behavior changes/breakages by skipping out on making new configs/placeholders only to be deprecated later, eg button_show_notification and hardcoded self.output.

You're almost there.

[maximbaz]

I don't want to tackle formatting in this PR, I don't like forever-hanging huge PRs with lots of unrelated changes. This PR is solving one specific problem - showing full output in a notification without refreshing module on click. Formatting needs to be redesigned in this module anyway: output needs to not be truncated, line_? needs to be introduced, color parsing needs to be improved, etc - all of this deserves to be done separately.

[lasers]

We're not tackling formatting. We're just adding output_full to a dict... doing same self.output = thing, but in different place... and with self.composite instead of string, and passing the composite to notify_user.. so we don't have to run same thing twice on every click.

[maximbaz]

I just don't see the benefit of doing this, at this moment it only complicates the code. When there are more formatter placeholders, of course it makes sense to pre-compute them all and store in a dict, and do some micro-optimizations like wrapping in a composite - but not right now when there are so many unresolved issues and formatting itself needs to be redesigned.

[lasers]

Just pushed out modifications. I think this does what you want... and it looks like we don't really need button_refresh... so we can hide it... and I was slightly off... I don't want you to pass a composite. I wanted you to make and use self.dict instead... so all format have same placeholders.

[maximbaz]

I think we do need button_refresh, I want the middle click (button 2) to be a silent refresh without any notification (because the notification it will show will contain outdated info anyway).

[lasers]

Sensible. I pushed it out. You can use format_notification = '{output_full}'... and the format support hidden {output_full} too. Things look okay on your side? Squash this PR?

[maximbaz]

You can squash during merge, on your side you should see a button "Squash and merge". It is undesirable to squash commits in PR itself, because later we will not be able to see the history, why we introduced what.

[maximbaz]

@lasers is this okay that this is now defined in post_config_hook and not above, among other parameters? It is also not documented

[lasers]

Okay because without on_click, the module still does not refresh on any other click events than 2 and we don't have any other button, so we don't need to mention it... and the behavior still will be same as before, but with optional notifications on click events (except 2) this time.

button_refresh is not needed and {output_full} is not documented, but this is implemented to do what you want... and having format_notification implemented correctly is what I want.

[tobes]

why is this feature hidden? It should either be documented or removed imho

[lasers]

New {output_full} is the true {output}.
... and {output} is the true {line_1} since it does output[0] in the code.

We keep {output_full} output as-is... for notifications, but will remove newlines for the bar (although undocumented).

And we should address #1265 PR too because I assume async_script could want {output_full} too... so maybe we should address all of this in one swift PR instead... or something like that.

Most All users are using {output} so we can't swap that. If we have to break something, I prefer we do it once instead of twice, thrice because of other PRs too.

[maximbaz]

For this one shift PR, you already have a prototype for rewritten external_script, right?

[lasers]

I did start something particularly to deal with explicit lines and to parse py3status's format in the output lines too instead of external_script's second line hidden color... I don't have Popen in there, and I pretty much stopped working on that already. See what @tobes want to do here.

@tobes He need {output_full} for his notifications and format_notifications are made to work with last (data), not current data like most modules... so I wanted to add notification (state) where users can specify what notification state to use (current, last) (default: current).

[tobes]

If it is used it should be documented :)

[maximbaz]

So still this confuses me, why None? Why not teach people that button 2 is for silent refreshes, since it's is a default in so many modules? Why force everyone to enable this?

[lasers]

button_refresh is used because some modules connects to Internet. We don't want people to scroll and start hitting up Internet.

It's in external_script, whatismyip, github, google_calendar.

[lasers]

Talking specifically about mouse scroll wheel. We can get 50+ events per second if we scroll hard.... I don't think it's good idea to force notifications on existing external_script users because we don't know their cases and most of them can not want it... so they have to fix their config to turn it off... Having it off (and always off by default) is better.

[maximbaz]

Well not true, it's just hiding the issue - those who enabled notifications will suffer from this issue. Lets introduce button_show_notification then

[lasers]

Why do we need button_show_notification if it works okay on several buttons?

[lasers]

Obviously we don't agree on same things.... :-)

Pinging @ultrabug for his opinion on this matter.

[maximbaz]

I guess I simply don't understand your use-case, with what you are proposing most buttons will not be doing anything (not even module refresh, absolutely nothing). Why not put some useful action on button click if these buttons are unused?

By analogy we can say that Chromium is forcing an action "open new window" when you press "Ctrl+N". This shortcut is unused, so they put some action on it. Same here, button is unused and I have a useful action that I can put on this button.

I'll update the code in this PR to have it show my intention for this, and yeah let's have a third opinion on it 🙂

[lasers]

Because this is external_script. It does not really have a specific purpose and your use case is a bit different in that you only wanted a notification on last output... External scripts often have some intervals in them... and format_notification might confuse users in thinking that they'll be getting a notification on every interval... (say I put in 7200, then I expect a notification every two hours). This is not the case... so I'm not sure if we should even add format_notification here too... It's too early to tell.

What you wanted is minimally implemented. Minimal is the keyword here. If users come by wanting notification on every two hours instead, then yes, it would be easier to implement that minimally too... using this same format_notification.

If we add buttons, then it's hard to undo them so I'm trying to avoid not adding unnecessary configs because if an user stop by with a PR containing something really useful for external_script and it requires a button, then by all means, we should let him do that.

external_script is hard in that it does not really have specific purposes... It might be better to enhance https://py3status.readthedocs.io/en/latest/configuration.html#special-on-click-commands to accept {placeholders} too, not just $OUTPUT...

And obviously, to enhance on_click too with new options to better address #1264.

[maximbaz]

if an user stop by with a PR containing something really useful for external_script and it requires a button, then by all means, we should let him do that.

ah, my feelings are hurt 😢, my PR is not considered cool/useful enough to make a case for reserving a button... 😛

Although I find it strange that we prefer to let notifications be shown on mouse scroll to creating a dedicated button_show_notification, I'm fine with conceding this to you — in reality there is a throttle on notifications, so even if you scroll extensively, you will get at most one notification every 5 seconds. Accidents will not be too painful.

However, I still believe that by default showing a notification on click is better than doing absolutely nothing. If you are annoyed by notification, no configuration change is needed — simply don't click on the module, click is not doing anything except showing a notification.

[lasers]

However, I still believe that by default showing a notification on click is better than doing absolutely nothing.

Doing absolutely nothing is the best way to ensure that we don't break user configs. Instead of trying to put something in the format_notification by default... only to later remove it., we break one thing. We can defer the format_notification to users instead.

This is external_script though. Users uses this to execute their scripts. We don't know their scripts. Their scripts could already contain some notifications too... so now they get dual notifications... One for clicking the module (and not caring about last output)... and one for when when their script is done doing something.

I believe there can be some grief to have notifications enabled by default versus none.

Still waiting for third opinion... No rush... Toy with the current module.

[maximbaz]

@lasers why is this line needed? it already contains full output since line 98, isn't?

I got it, before this the output was joined by space...

[lasers]

To remove newlines, put them all on one same line for the bar.
And to preserve newlines for the notification.

Try cal.

[maximbaz]

@lasers I've just found something out... if I define on_click 1 = ... in .i3status.conf, this does not override on_click in the module itself — do you know if this is intentional? I assume it is, but I didn't know this.

Because of this behavior, of course it doesn't make any sense to have notifications be enabled by default, it is very confusing to define a custom on_click and still see notifications popping up. I think now the code now matches what you and I want, we don't need to wait for the third opinion anymore 🙂

[lasers]

do you know if this is intentional? I assume it is, but I didn't know this.

I don't know if it is intentional. @tobes is much more qualified to answer this than I am. Also, maybe it had something to do with #1270 PR. I can try to look around maybe later.

Also, I recall @tobes was working on something to enhance button functionality so we can specify functions too... for more customization and/or richer experience. Overriding functions could be one of them. Also, it's possible that they both have same name and had nothing to do with each others.

[maximbaz]

It most probably is intentional, because on_click() function is called for all button clicks, while on_click 1 = ... in .i3status.conf is there to define command only for one specific button, it would have been bad if that completely shadowed the entire on_click() function possibly disabling other buttons.

So, I'm happy with merging it as it is, have another look on your side if it looks like you want as well and let's move forward 😉

[maximbaz]

Fixed documentation here, {output_full} still remains undocumented as per your request.

[cyrinux]

Why don't document this?

[lasers]

Documented. 😭

[maximbaz]

A friendly ping @lasers, anything else you want to see implemented before this can get merged? I think it currently does as you wanted it to work.

[lasers]

I hold off on this mainly because I think we should try external_script: new module from scratch to address #1265 too and that format_notification only work with last notification... and it only works when we clicks too... instead of normal notifications, ie battery_level, pomodoro, wwan.

We need to make another config notification (state) to better support current use cases in external_script even if it's only one placeholder right now... number of lines...

This would be similar to the new state config in do_not_disturb which is implemented nicely and not hardcoding format_notification to something like do_not_disturb's last state.

---

Yes. format_notification is ideally implemented, but I'm going to wait until we get notification config too with the following options because it's better to wait until we support this too... instead of pushing merge button and forget about it immediately. 🙂

• current: Display notifications based on formatted placeholders (and not duplicate notifications, but only if something changed in the output between last output and current output... and also respecting formatted line, eg \?if=line>750 hi the compilation may be almost done, send a notification. This would behave same as wwan.

• last: Simply show the notification of last output and/or formatted placeholders without refreshing.

This PR deals with your last usecase. I want this to work with current usecase too.

[maximbaz]

OK if you want to play with rewriting external_script, I would be happy to test, please just don't forget about it, I don't want to use my own fork for the next half a year 😉

[lasers]

I implemented notification_state. This option will accept current or click. Is notification_state a good config name to use? Alternative name could be just state or notification. Anyway, this state part is done... Test this with your script and hopefully we can get this PR merged soon enough next.

[tobes]

I implemented notification_state.

This just makes the PR too complicated imho, also there is no commentary about why you add this.

can we just do the 'one thing == one PR'

[lasers]

@tobes format_notification is (often) used for current data. @maximbaz have a different goal of printing the notification about last data without triggering a refresh. This can confuse users about format_notification's purpose.

I introduce notification_state = 'current' to be consistent with format_notification's behavior in that it only works with current data... and still be able to do what @maximbaz want to do here.

If I take it out, then it will only work with last data without refreshing. If we need to do 'one thing == one PR', then we need @maximbaz to forgo his goal and have him implement this with current data.

[maximbaz]

Having notification_state = 'click' is fine for me, @lasers do you still plan to re-write this module or you are OK with this PR?

[lasers]

@maximbaz I'm leaning toward Do Not Rewrite so I'm OK with this PR since you added what you wanted (post-run click notifications) and I added what I wanted (proper notification support + options/placeholders) along with examples to clarify notification states. This PR should be out of our hands for weeks.

[tobes]

I think we need a more generalised solution to this issue see #1531

[maximbaz]

@lasers just want to confirm with you, since you were driving implementation of notifications in this PR, are you interested in taking on the work of moving this to core? As far as I understood, the idea is that notifications object will be supported by all modules simultaneously, and all that will remain in this PR is the addition of {output_full} format placeholder.

[maximbaz]

Just a reminder for future self: this throws exception if notification object doesn't contain click key

[lasers]

What configs? I'm not able to reproduce. The notification dict should already have all three.

py3status/py3status/modules/external_script.py

Lines 111 to 114 in c6a1c21

	self.notification = {'normal': []}
	if self.notifications:
	for x in ['changed', 'click', 'current']:
	self.notification[x] = self.notifications.get(x, False)

[maximbaz]

This is my exact config:

order += 'external_script date'

external_script date {
  cache_timeout = 60
  script_path = 'date +"%a, %d %b"'
  format = 'Calendar: {output}'
  on_click 1 = 'exec gsimplecal'
}

Add this, then restart i3 (important), then click on the module.

I get an error, and in journalctl I see

py3status[12014]: on_click event in `external_script date` failed (KeyError) external_script.py line 199.
py3status[12014]: on_click event in `external_script date` failed (KeyError) external_script.py line 199. Please try to fix this and reload i3wm (Mod+Shift+R)

[lasers]

@maximbaz I'm not sure what to do here. It might be more fun for you to try it first. I have my py3status things that I want to do first since they're all finished or nearly finished.

I think it would not be supported by all modules simultaneously and would behave much similar to thresholds where we have to add this to each modules that wants it. Wild guess, we need two functions... First to validate what we need (for self.notifications) in post_config_hook, then second to ... put them in good places... similar to threshold_get_color. I can give it a shot eventually.

[maximbaz]

@lasers could you describe what is happening in this if block?

[lasers]

post_config_hook checking to find out which notifications to use... so we use a dict+list of booelans instead of repeatedly testing the conditions on each interval.

[maximbaz]

I'm looking at this right now and I think it's a bit over-engineered, both external_script and battery_level (and potentially updates) only need notifications on click (battery has extra special case of showing notification on certain threshold), is there any module that currently shows notifications always or when changed?

[lasers]

IIRC I think battery's implementation of showing notification on clicks is a bit silly. We likely could make it show up on the bar instead. Maybe with a toggle to show/hide what we want.

Current and changed is similar. I'm not even sure if it makes sense to make a changed state... because we technically could make changed placeholder so you do \?if=changed {test} in current state. Maybe better not to. Fleshing out a nice notification solution is hard though.

• wwan on changed.
• github on always (always failing). Could be in post_config_hook instead.
• battery_level is both click and changed.
  pomodoro is changed when timer reached zero. Not sure how we would add into this as it's not changed.

Things to take from this is that we are only making notification states to allow different kind of notifications to happen in a module. I guess we could use changed here.

• graphite seems to be always.

[maximbaz]

• wwan on changed - 👍
• github is not always, it's a weird way of showing error, completely custom use of notifications and should not be generalized
• battery_level is click and on certain thresholds, this is again custom and is not "changed" (i.e. it will not show notification when changing from 81% to 80%).
• pomodoro yes, it's again not changed but some custom logic.

I'm not sure how much we will be able to generalize if every module has some special use of notifications...

[maximbaz]

Still cannot grasp this logic, if notification is added for "changed", we add it to "normal"? What is normal?

[lasers]

With only two, it would be just if current or changed:.
With only 10 or 100, it would be just if current or changed or .. or .. or .. or .. False:

I add "True" to the list and we check the list itself to see if it's empty or not... instead of going through the list to find a first "True". When we go in, we now have (already optimized) list.

It's a bit different here because of notifications so we check both if statements. In other scenarios, we might use that list to loop through something.

Normal is both current+changed, but I don't want to say current. Idk.

[maximbaz]

This thread became too complicated and it's hard to get feedback on it. Let's start again, with a simple patch that just solves my need: #1585