module: add background and border colors #1486

lasers · 2018-09-02T12:40:05Z

This only works on i3-gaps. I add background and border colors as well as border_{top, bottom, left, right} widths. The border widths only work if you specified border.

lasers · 2018-09-02T12:55:11Z

py3status/module.py

+                    err += 'should be a string'
+                    raise TypeError(err)
+                self.py3_module_options[name] = Py3(self)._get_color(param)
+                self.block_options.append(name)


@tobes Look at this part. I don't know if Py3(self)._get_color(param) is ideal.

yes this is very bad

a) Instantiating Py3 just to access a function is a performance overhead

b) class methods starting with an underscore _ are private and should not be accessed outside of the class

I'll make a PR to make the function globally available

I'll wait for this. :-)

tobes · 2018-09-02T13:19:55Z

py3status/module.py

+                if not isinstance(param, basestring):
+                    err = 'invalid "{}" attribute, '.format(name)
+                    err += 'should be a string'
+                    raise TypeError(err)


I'd do this to be more readable

Also the invalid -> Invalid and using backticks ```` (screw github) not quotes "

raise TypeError( 'Invalid attribute `%s`, should be a string' % name )

but maybe we should also tell the user a little more info so they can better resolve the problem ie show them what we got

There is another example of this in this PR which you can fix too

lasers · 2018-09-02T16:29:49Z

This is right now. Something changed. I'm not sure what's going on. :-)

EDIT: The screenshots above are 4 months old.

lasers · 2018-09-02T18:37:42Z

@tobes Fixed. Ready for review. I included param in the errors too.

tobes

Ok these are some thoughts. I've not even looked to see how it affects the other code so there may be lots more changes ahead.

This is just minimal quality control not looking at the logic

py3status/module.py

tobes · 2018-09-02T20:40:35Z

py3status/module.py

@@ -311,7 +311,7 @@ def set_module_options(self, module):
        https://i3wm.org/i3status/manpage.html#_universal_module_options
        """
        self.i3s_module_options = {}
-        self.py3_module_options = {}
+        self.py3_module_options = {'block': {}}


this is ugly, I think we can do better

Used to be self.block_option. It's not going to be used for anything else other than background, border + its widths. Happy to hear your suggestion.

The whole dict is this.

self.py3.module_options = { 'block': { 'background': '#fff', 'border': '#fff', 'border_left': 1, 'border_right': 1, 'border_top': 1, 'border_bottom': 1, }, 'min_width': 15, 'align': 'left' }

is there anything special about the block stuff? If not why not just all live happily together?

We keep min_width and align to ourselves since i3bar is not equipped to handle more than one composite. That's why i3status is always colorized with good or bad.

Nothing special. I put them in its own dict for looping... I don't need to do anything with min_width and align and I wanted to keep custom (ie py3status and i3-gaps) options inside a dict instead of separate parameters. i3 universal options is in that other dict.

Also, if users didn't specify anything, the block is empty so I'm not wasting time looping through nothing. Can be background only.. or border stuffs only... or both of them.

whatever this needs a reworking at some point to make it more efficient/readable so I can live with it but make a comment explaining why this is in the code so the reasoning isn't lost

Left a comment.

I have separated them to make it more readable. Three dict options. py3status dict will not have more than two options max.

tobes · 2018-09-02T20:41:59Z

py3status/module.py

@@ -325,7 +325,7 @@ def set_module_options(self, module):

            if 'align' in mod_config:
                align = mod_config['align']
-                if not (isinstance(align, basestring) and
+                if not isinstance(align, basestring) and (


why have you done this?
explain your working and why is this not in the PR description

~~Travis.~~ Flake8.

I don't see why it is part of this PR maybe I've missed something
and please don't resolve issue I raise I'm not happy

Also I don't have to help you with your code I can just NAK it

../module.py|328 col 25 warning| line break after binary operator [W504]

We have this in tox.ini to skip those.

[pytest] flake8-ignore = # line break after binary operator (2018-Aug-22) W504 # invalid escape sequence (2018-Aug-22) W605

When we black, we can remove W504 later so Travis will complain next time. W605 is complaining about several \?if=. I'm just seeing this warning here and it didn't take much to remove the warning. I'll undo this.

but wtf is this being done in this PR for - how many times do I have to say the same shit?

Because that's how I fixed errors/warnings in first place. I forget I had this blacklisted.

py3status/module.py

tobes · 2018-09-02T21:27:45Z

py3status/module.py

@@ -325,7 +325,7 @@ def set_module_options(self, module):

            if 'align' in mod_config:
                align = mod_config['align']
-                if not (isinstance(align, basestring) and
+                if not isinstance(align, basestring) and (


I don't see why it is part of this PR maybe I've missed something
and please don't resolve issue I raise I'm not happy

Also I don't have to help you with your code I can just NAK it

tobes · 2018-09-03T22:04:48Z

py3status/module.py

+
+                if name != 'border':
+                    continue
+                borders = ('top', 'bottom', 'left', 'right')


I'd just prefer the real list than building them straight after, but not a biggy.

Why tuples not lists?

Faster? I don't need to touch the tuples.

please answer the question.

If you don't give me what I need how can I do a review? Also please try to review your comments I find some very hard to follow. Other times you can be clear so you have the skill if you are bothered, please be.

I'm serious. Is it not faster than the list? Immutable. I'm using it for loops so I don't need to manipulate the list.. That's what I meant by not needing to touch the tuples.

Maybe it does not matter because it does not apply to the line 369, but the next line down... Should be used with tuples, not list. Meh.

please use a list it is clearer and no real other difference here. If you want to save cycles there are better ways - readability wins over pretty much anything else.

Okay, I'll switch to list right now.

tobes · 2018-09-03T22:06:12Z

py3status/module.py

+                if not isinstance(param, basestring):
+                    raise TypeError(
+                        'Invalid attribute `{}`, '
+                        'should be an int. Got `{}`'


I think you did a copy paste string not int but really should be color

tobes · 2018-09-03T22:11:27Z

py3status/module.py

+                )
+
+                if name != 'border':
+                    continue


what's going on here? I think you should just end the block here and unindent the following code seeing as you then screw the name variable over.

background and border have identical code. I'm looping with two names. If users entered something... and it's background, continue. If it's border, then we make a quick list of borders, then start checking for inputs from users. Fine with screwing the name variable because I'm finished with border part (code is already added in block). Am working on getting border widths (int) at this time.

Do you read what I write?

I'm pretty sure it's wrong - yeah it runs but it is messy for no reason and is a maintenance nightmare. If you are doing something clever where is the comment.

Have a fun evening I'm done :)

You always say "What's going on here?" I explained it, but I know you know what's happening here... so why are you asking me that question? Maybe no question next time so I don't have to answer your question.

That's all I did... answering that question. I did read what you said. ;-) EDIT: Gnight! :)

EDIT: Answering your question. Yes, I did read what you wrote. See what I said above.

Done. :-) I keep ~~tuples~~ list because it looks ugly / out of place if I list em all.

ok so the code was ok except it had no comment.
github sometimes makes it hard to see the actual code

but this is much clearer so I prefer it. Clarity is everything - make your code easy to read for idiot reviewers and your life is easier too

tobes · 2018-09-04T09:41:18Z

py3status/module.py

        self.i3s_module_options = {}
        self.py3_module_options = {}
+        self.gaps_module_options = {}


looking much better now

self.gaps_module_options I don't like this name self.i3_gaps_module_options?

Also can the comment be by the definition not all above everything. I should not have to scan the code to see things - it should all be super clear.

As an aside - I think background is fine with classic i3-bar for some time now (border is there but has issues so not practical)

we still need the _get_color(border) fix and do you also want the alpha channel?

# keep i3-gaps module options in gaps module options. this is wrong should it not be # module options only available in i3-gaps

I made variable names more clear enough to not warrant a comment.
EDIT: Yes on alpha channel. More nice for users if we can add this too.

https://github.com/Airblader/i3#transparency--rgba-colors

Alpha may be already available with i3bar -t. The exception is that this wouldn't work with CSS color names and general colors unless you have a solution in mind for those particular names. We also could support rgba() in the future. I think users could be more likely to use color hex over rgba() so maybe we don't really need to add anything here.

@tobes Ping... How goes the _get_color() fix since it's the only thing preventing this PR from getting merged... I'm also okay with forgoing alpha channel if it's not an easy fix. Thanks.

lasers · 2018-11-09T20:42:22Z

@maximbaz You should try this next!!!!

order += "static_string left"
order += "static_string center"
order += "static_string right"
order += "static_string block"
static_string left {
    format = 'l[\?color=good&show e]'
    format += '[\?color=lightblue&show f][\?color=red&show t]'

    border = '#EB709B'
    position = "left"
    min_length = 20

    border_top = 2
    border_bottom = 2
    border_left = 2
    border_right = 2
}
static_string center {
    format = '\?color=degraded center'
    format = 'c[\?color=degraded&show e][\?color=pink&show n]'
    format += '[\?color=orange&show t][\?color=gold&show e]r'

    border = 'orange'
    background = '#005577'
    position = "center"
    min_length = 20

    border_top = 2
    border_bottom = 2
    border_left = 2
    border_right = 2
}
static_string right {
    format = '\?color=bad right'
    format = 'r[\?color=bad&show i][\?color=lightgreen&show g]'
    format += 'h[\?color=lightblue&show t]'

    border = 'lightgreen'
    min_length = 20
    position = "right"

    border_top = 2
    border_bottom = 2
    border_left = 2
    border_right = 2
}

static_string block {
    format = '\?color=red ❤'

    min_length = 20
    background = 'gold'
    position = 'center'
    border = 'white'

    border_top = 2
    border_bottom = 2
    border_left = 2
    border_right = 2
}

maximbaz · 2018-11-09T21:11:06Z

It's fun no doubt 🙂

I don't see how I personally will use this feature yet, but as I said in the other ticket, in principle I'm not against this PR because it enables functionality that i3bar (or i3status? I'm confused a bit) already supports, currently py3status prevents using some options that what i3bar/i3status provides.

@su8, just curious, I see you upvoted this PR, could you tell us if you are going to use this feature? Maybe you already know how exactly, for which modules? I'm asking because this has been a point of discussion, whether there are some actual use-cases for this PR.

lasers · 2018-11-09T22:40:29Z

Users don't have to use all borders. They can set 0 for all borders except top or bottom and each module can have its own top/bottom color. The next PR (global options) will allow users to set border_{top,bottom,right,left} once in py3status {} instead of all modules then on said modules, they can override it with new int value. (much fewer configs).

...enables functionality that i3bar (or i3status? I'm confused a bit) already supports, currently py3status prevents using some options that what i3bar/i3status provides.

(i3bar). This works with i3-gaps. This works brokenly with i3 too. py3status passes the options and we validate/apply the options to each composite with some exceptions.

@su8 does not use i3wm+py3status. @su8 is a good sport too. <3

maximbaz · 2018-11-09T22:52:26Z

i3-gaps is widely popular, so it's okay.

Anyhow, if you wanted my feedback, you have it now: yes for this PR (because it allows using already existing i3bar features) and no for updates module (because it's just a special case of external_script, which already provides the same functionality, plus #1439 makes external_script even more feature-rich).

ultrabug · 2018-12-23T23:20:25Z

Ok @lasers the PR is now thin and I have found a way to get rid of the get_color function as you can see in my added commit.

Could you test it one last time? It's working for me, so merge is soon to come.

obsolete

lasers · 2018-12-23T23:25:06Z

@ultrabug It's not working for me.

2018-12-23 17:23:43 INFO loading module "updates" from py3status.modules.updates
2018-12-23 17:23:43 INFO Module `updates` could not be loaded
2018-12-23 17:23:43 INFO No module named 'py3status.modules.updates'
2018-12-23 17:23:43 WARNING Loading module "updates" failed ('NoneType' object has no attribute 'py3'). (AttributeError) mo
dule.py line 372.
2018-12-23 17:23:43 INFO Traceback
AttributeError: 'NoneType' object has no attribute 'py3'
  File "py3status/py3status/core.py", line 532, in load_modules
    my_m = Module(module, user_modules, self)
  File "py3status/py3status/module.py", line 96, in __init__
    self.set_module_options(module)
  File "py3status/py3status/module.py", line 372, in set_module_options
    color = self.module_class.py3._get_color(background)

EDIT: If module's get_color(color) function gets removed... We get 'py3status.constants.COLOR_NAMES' imported but unused [F401] so COLOR_NAMES ought to go too.

EDIT: Nevermind. Travis flake8 caught this. You see it now.

ultrabug · 2018-12-23T23:34:53Z

@lasers hmm I can't reproduce this and it shows on a module that's not part of the project, can you repeat the problem on other modules?

lasers · 2018-12-23T23:46:24Z

I don't see this problem on my usual modules. The colors is working now without pew and updates. After commenting out pew, I can't reproduce this error with updates.

ultrabug · 2018-12-23T23:53:36Z

Why then? :(

lasers · 2018-12-24T00:03:31Z

I don't have a clue. It is just pew so far. I uncommented several lingering module configs and can't reproduce this error anywhere else. I enabled pew and updates at bottom of the config and it worked fine now. Was it caches or something?

lasers · 2018-12-24T00:32:20Z

@ultrabug If you find . -name __pycache__ in py3status and delete them, does pew work for you now?

lasers · 2018-12-29T11:18:10Z

@ultrabug Hi. I have 6 open PRs... Plz get this junk out of here today. 🔥🗑️

ultrabug · 2018-12-30T17:11:53Z

Thanks, works fine for me

lasers · 2018-12-30T17:14:19Z

@ultrabug With this merged, do you want me to reopen/fix up #1481?

ultrabug · 2018-12-30T17:15:22Z

@lasers if you make it a simple and one-thing-at-a-time PR, why not

lasers · 2018-12-30T17:23:22Z

I don't know about making it simple. It's all-in because it's much easier to change background for all modules with one config in general {} instead of repeatedly setting this in each module. Same with borders. I might as well go all the way with all i3bar/i3bar_gaps options to make sure we don't miss out on anything.

ultrabug · 2018-12-30T20:54:38Z

Then just open a PR that says this clearly. TBH the previous one was messy to me.

lasers added the enhancement 😍 I have created an enhancement label Sep 2, 2018

lasers commented Sep 2, 2018

View reviewed changes

lasers changed the title ~~module: support background and border colors~~ module: add background and border colors Sep 2, 2018

lasers force-pushed the bg-and-border branch from 2443b4c to 20af299 Compare September 2, 2018 12:58

tobes reviewed Sep 2, 2018

View reviewed changes

lasers force-pushed the bg-and-border branch from 20af299 to 2b608ab Compare September 2, 2018 15:01

lasers force-pushed the bg-and-border branch from 2b608ab to 3204fa6 Compare September 2, 2018 18:36

lasers force-pushed the bg-and-border branch from 3204fa6 to 606c63f Compare September 2, 2018 18:39

tobes suggested changes Sep 2, 2018

View reviewed changes

lasers force-pushed the bg-and-border branch 4 times, most recently from 65a98e0 to b4b28ab Compare September 2, 2018 21:21

tobes suggested changes Sep 2, 2018

View reviewed changes

lasers force-pushed the bg-and-border branch from b4b28ab to e1b341d Compare September 2, 2018 21:40

tobes previously requested changes Sep 3, 2018

View reviewed changes

lasers force-pushed the bg-and-border branch 7 times, most recently from 0274a1c to f529fba Compare September 4, 2018 08:49

tobes reviewed Sep 4, 2018

View reviewed changes

lasers force-pushed the bg-and-border branch 3 times, most recently from 02165f1 to 3c5e133 Compare September 4, 2018 13:06

lasers force-pushed the bg-and-border branch from 3c5e133 to b4e6f1a Compare September 29, 2018 11:35

lasers force-pushed the bg-and-border branch from c9936e2 to 93838ec Compare November 5, 2018 03:55

lasers force-pushed the bg-and-border branch from 66843dd to 74372bb Compare December 23, 2018 18:09

module: add background and border colors

72420f0

lasers force-pushed the bg-and-border branch from 74372bb to 72420f0 Compare December 23, 2018 18:10

lasers mentioned this pull request Dec 23, 2018

module: add more module/global options #1481

Closed

ultrabug force-pushed the bg-and-border branch from f55a5b1 to 0cafade Compare December 23, 2018 23:21

use generic py3._get_color

4d3e0df

ultrabug force-pushed the bg-and-border branch from 0cafade to 4d3e0df Compare December 23, 2018 23:33

ultrabug merged commit 0992f46 into master Dec 30, 2018

lasers deleted the bg-and-border branch December 30, 2018 17:14

lasers mentioned this pull request Dec 30, 2018

modules: don't set module options on failed modules #1608

Merged

maximbaz mentioned this pull request Jan 14, 2019

module: add urgent background and borders #1627

Merged

module: add background and border colors #1486

module: add background and border colors #1486

Conversation

lasers commented Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobes Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

lasers commented Sep 2, 2018 • edited Loading

lasers commented Sep 2, 2018

tobes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasers Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

lasers Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasers Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasers Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasers Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

lasers Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobes Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

lasers Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

lasers Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasers commented Nov 9, 2018 • edited Loading

maximbaz commented Nov 9, 2018

lasers commented Nov 9, 2018 • edited Loading

maximbaz commented Nov 9, 2018

ultrabug commented Dec 23, 2018

lasers commented Dec 23, 2018 • edited Loading

ultrabug commented Dec 23, 2018

lasers commented Dec 23, 2018 • edited Loading

ultrabug commented Dec 23, 2018

lasers commented Dec 24, 2018

lasers commented Dec 24, 2018

lasers commented Dec 29, 2018

ultrabug commented Dec 30, 2018

lasers commented Dec 30, 2018

ultrabug commented Dec 30, 2018

lasers commented Dec 30, 2018

ultrabug commented Dec 30, 2018

lasers commented Sep 2, 2018 •

edited

Loading

tobes Sep 2, 2018 •

edited

Loading

lasers commented Sep 2, 2018 •

edited

Loading

lasers Sep 3, 2018 •

edited

Loading

lasers Sep 3, 2018 •

edited

Loading

lasers Sep 2, 2018 •

edited

Loading

lasers Sep 3, 2018 •

edited

Loading

lasers Sep 3, 2018 •

edited

Loading

lasers Sep 3, 2018 •

edited

Loading

tobes Sep 4, 2018 •

edited

Loading

lasers Sep 4, 2018 •

edited

Loading

lasers Sep 19, 2018 •

edited

Loading

lasers commented Nov 9, 2018 •

edited

Loading

lasers commented Nov 9, 2018 •

edited

Loading

lasers commented Dec 23, 2018 •

edited

Loading

lasers commented Dec 23, 2018 •

edited

Loading