lasers / py3status

2 stars 0 forks source link

I'd like your input re: the new format, @lasers #5

Closed girst closed 6 years ago

girst commented 6 years ago

Hi @lasers, we recently got to talk about the new format a bit in the wwan pull req., and you'll remember that our opinions about said topic were 180° different. As a contributor to a project I really care about I'd like to have a little bit more formal discussion about it.

I know you've put a lot of work into the new format and I appreciate that, and that's why I want you to have a look at my proposal first and let me know your thoughts about it. Once again; I'd love to continue this conversation over email and please understand that I do not want to pass over you in this issue, and work together to get to a solution everyone can live with.

greetings,
-- gir

---(Proposal starts below)---

My issues with the new (complicated) format

Hi, I want to throw a few words out about the new highly customizable format. With this issue, I'd like to generate some discussion about the direction py3status is headed.


Like most, I come from using i3status for a while, and was drawn to py3status because it felt "the same". By that I mean the configuration for all the py3-modules was generally very similar to a i3 one.

Recently, many of the new (and newly updated) modules are using a single conditional format. While it is meant to provide more flexibility, it also introduces a lot of complexity the end user has to manage.

Since I've been a little bit involved with the wwan.py (formerly wwan_status_nm.py) module, I'll base my thoughts on its code/format.

Take a look at how it used to be: you'd specify very simply how you want the network to be displayed when it's working, how when it's not, and when there is an error.

format_up = "Mo: {ip} {operator}/{netgen} ({signal}%)"
format_down = "Mo: down"
format_error = "Mo: down"

The new system reminds me of regular expressions: There are a whole bunch of brackets, question marks and backslashes and the significant whitespace makes indenting impossible.
Also, the correct semantics are only documented in the developer's guide, but nowhere for the user.

Most of the things one could configure won't be used in practice and what used to be simple now requires much more work. (the code below is from lasers -- I couldn't get the new thing to behave like the old myself)

format = '[\?color=state Mo: [\?if=state=11 {format_ipv4} '
format += '{m3gpp_operator_name}/{access_technologies_name} '
format += '({signal_quality_0}%)|down]]'

This is why I believe this change is unfriendly to both new and old users:

Finally, I would like to suggest a compromise: Return to multiple simple format_* strings, but provide a {advanced} placeholder that will be replaced with the customizable format.

[modeline]: vim: syntax=markdown

lasers commented 6 years ago

I know you've put a lot of work into the new format

As far as I know, this is @tobes's baby. I'm seizing and loving the new power. I do not have the final say. @ultrabug and @tobes does. I do agree with @tobes that this simplifies lot of things and we probably still have some way to go. I think we should go for it. The old format or multiple formats can bring some difficulty in breaking up things or combining things.

With the new format, users can (I said can) have the ability to re-define what is format_down, what is format_error, etc. For instance with volume_status... Somebody did ask how to make volume_status go away if the percentage is in 15% - 90% range. This was easy enough with the new format without making a pull request.

Solution.

order += 'volume_status'
volume_status {
    format = '[\?if=percentage>15 [\?if=percentage<90 '
    format += '[\?if=is_input 😮|♪]: {percentage}%]]'
}

The old format likely would only expose more configs like low_hidden_threshold, high_hidden_threshold.... then more configs when users want this to act a bit different because he plugged in a headphone... or etc cetera.

The multiple format, I think, would bring more complications and issues than the new format... and requires users to make pull requests. By putting everything in the format, then we can have fewer pull requests... I think it would be better if we can say the issues lies in the format this time inside of in the code.

I can help you out with your format. I don't know what your issue with wwan is... IIRC we keep wwan simple like i3status's wlan and ethernet where it only reported down when not connected. I can help you out with your format, but yes, you're right. We don't have documentations for this.

I like having non-specified format where basically anything goes. Naming good names for config, placeholders, etc is always hard too. I initially was not for this complicated format, but later changed my mind when I keep playing with this new format stuffs.... and made many cool things in i3 manipulation module PR. I don't know if you can test it, but you should try some of the examples if you can.

Looking at the wwan conversation, I think this might be it.

    format = '\?color=state WW: [\?if=state_name=connected '
    format += '({signal_quality_0}% at {m3gpp_operator_name}) '
    format += '[{format_ipv4}][ {format_ipv6}]|'
    format += '[\?if=state_name=registered disconnected|{state_name}]'

If it's not, please tell me what went wrong and soon enough, we will have the format you want... The next person will want it differently, then that just leads to a messy code.

Putting everything in the format and data allows us to simplify things like this one.

This PR https://github.com/ultrabug/py3status/pull/1132/files might be too simple of an example, but putting everything in the format and data allows us to simplify many things, to remove many unnecessary lines, and by doing that, we have good chance of reducing bugs too.

I think it also will be hard for me to go back to old way of not combining everything because the new questions would be... How should we break things up? What should be broken up?

Finally, I would like to suggest a compromise: Return to multiple simple format_* strings, but provide a {advanced} placeholder that will be replaced with the customizable format.

I personally this is better than the old way. You should talk about this issue with @tobes and @ultrabug too... and anybody else involved in py3status community.... as it would be easier to fix your format than to fix the code especially if we knew we fixed up the code to stay minimal, does not do anything more than to bring out the placeholders, threshold colors, etc.

Just to be pedantic, thewwan format is slightly different than here.

    format = ('\?color=state WW: [\?if=state_name=connected '
              '({signal_quality_0}% at {m3gpp_operator_name}) '
              '[{format_ipv4}][ {format_ipv6}]|{state_name}]')

I hope with enough documentation and examples, we can overcome this.

lasers commented 6 years ago

lasers... that this simplifies lot of things and we probably still have some way to go.

I think ideally to help making things more better, we need to work on more helpers... For instance with wwan, we could make one to deal with ... erm, format_thresholds (?) or such... to expose this:

        self.network_states = {
            -1: 'failed',
            0: 'unknown',
            1: 'initializing',
            2: 'locked',
            3: 'disabled',
            4: 'disabling',
            5: 'enabling',
            6: 'enabled',
            7: 'searching',
            8: 'registered',
            9: 'disconnecting',
            10: 'connecting',
            11: 'connected'
        }

... as a config. Maybe expose others too... eg self.network_speed, self.registration_states and self.modem_bands for easier string/format overriding.

    format = ('\?color=state WW: [\?if=state_name=connected '
              '({signal_quality_0}% at {m3gpp_operator_name}) '
              '[{format_ipv4}][ {format_ipv6}]|{state_name}]')

    states_or_something  = {8: 'disconnected'}

... so you can have {state_name} print disconnected instead of default registered and this would help with i18n too. This likely would work better in music players too. cmus example.

    format = '[\?if=is_started [\?if=is_playing > ][\?if=is_paused \|\| ]' +\
        '[\?if=is_stopped .. ][[{artist}][\?soft  - ][{title}]' +\
        '|\?show cmus: waiting for user input]]'

simplified to... (new default cmus)

    format = '\?color=state {format_state} [[{artist}][\?soft  - ][{title}]'
    format += '|\?show cmus: waiting for user input]'

    format_thresholds = {
        'state': [
            (0, '..'), (1, '>'), (2, '||')
        ],
    }
    color_thresholds = {
        'state': [
            (0, 'bad'), (1, 'good'), (2, 'degraded'),
        ],
    }

If users don't really change much from default... Their new format could be just this.

    format = '[\?color=state {format_state} [{artist}][\?soft  - ][{title}]]'

By helping wwan module with one thing, we inadvertently help other modules too. So it's good to make and use helpers everywhere if possible... And if there are a bug in helper, fixing that will fix all modules that uses it... This just simplify things. I also have a helper or two lined up.

This particular single helper that hasn't been made by anyone yet... can simplify the following modules: cmus, moc, deadbeef, mpris, mpd_status (w/ [play], [pause], [stop]), and technically on-off modules like DPMS, file_process, etc too. (which we don't have to do because a quick if condition in format already works fine). Maybe battey_level and kdeconnector could use it too for their states.

Really, there already are lot of modules so I think it would be in our interests in simplifying things more if possible. Changing things just sucks balls. Idk. ¯\(ツ)

cyrinux commented 6 years ago

I like new format possibility offer, I admit less readable, but powerfull, but I don't want format war ¯(ツ)/¯

lasers commented 6 years ago

RE: new simplified cmus format.

    format = '\?color=state {format_state} [[{artist}][\?soft  - ][{title}]'
    format += '|\?show cmus: waiting for user input]'
    format_thresholds = {'state': [(0, '..'), (1, '>'), (2, '||')]}

Also, if we make this new helper mentioned earlier, we could make something much closer... by flipping everything here upside down to achieve the old formats.

cmus {
    # old formats does not allow selective colors, so we keep color "hidden" here
    format = '\?color=state {format_state}'

    # using states as old formats
    format_thresholds = {
        'state': [
            # acts like format_stopped
            (0, '.. [[{artist}][\?soft - ][{title}]|\?show cmus: waiting for user input'),
            # acts like format_playing
            (1, '> [{artist}][\?soft - ][{title}]'),
            # acts like format_paused
            (2, '|| [{artist}][\?soft - ][{title}]'),
        ],
    }
   # default color thresholds, not likely to be here in user configs.
    color_thresholds = {
        'state': [
            (0, 'bad'), (1, 'good'), (2, 'degraded'),
        ],
    }
}

Notes: cmus have 2 stopped states: Stopped with and without song metadata so the state is fine. We could make another state for one without song metadata... which I guess could be state = -1... or if users don't care about this particular string, then it can display .. instead.

If some users does not like only .. in cmus old formats, then we can get a new PR with a new config format_stopped_no_song to deal with this... which could be bad and maybe we want to avoid that kind of issue with everything else too.

As for the helper, this only work if we add it to all modules (not likely), but we could add it to wwan, pomodoro, bunch of song players, and anything else with state numbers to somehow simplify things for users. I think to really simplify absolutely everything, we would put it all in the format.

That's also a problem itself because of users concern (like yours, thank you for expressing) and having everything there will just make format ugly and utterly complicated.

girst commented 6 years ago

Hi lasers, thanks for your input. I didn't have time yesterday or today to read it in the detail it deserves. I'll get back to you in the following days!

lasers commented 6 years ago

I'm closing this. Feel free to talk more. Also, I believe I'm finished pomodoro: new module from scratch with one format. It is close to default pomodoro format with minor tweaks to show current pomodoro and non-total time. 1500 seconds does not feel same as 25:00.

Users can also customize notifications with the formatter (or disable it outright). I made few example themes too. More/missing placeholders were exposed and hardcoded stuffs were removed in favor of putting them into the formatter code instead. One annoying bug about pomodoro is that because of hardcoded ceiling time, we would get notification a second early before the timer is up. I think this is more true to the time now that I need to check numbers too to avoid -1:59:59.

One nice thing about new pomodoro is that with cache_timeout = 60, I won't notice I'm on pomodoro session versus numbers always counting down which, I think, is a distraction for me.