Widgets from default config are alive while is not used #251

Closed
ei-grad opened this Issue Dec 26, 2012 · 28 comments

3 participants

@ei-grad

In #185 I had found that there two instances of the Clock widget in running qtile, but I create only one clock instance in my config. The second instance comes from default_config and it is not good.

It is in current develop branch head.

> /home/ei-grad/repos/qtile/bin/libqtile/widget/clock.py(32)__init__()
     31         import ipdb; ipdb.set_trace()
---> 32         self.update()
     33 

ipdb> where
  /home/ei-grad/repos/qtile/bin/qtile(89)<module>()
     87 if __name__ == "__main__":
     88     locale.setlocale(locale.LC_ALL, locale.getdefaultlocale())
---> 89     q = make_qtile()
     90     try:
     91         q.loop()

  /home/ei-grad/repos/qtile/bin/qtile(76)make_qtile()
     74     log = manager.init_log(log_level)
     75 
---> 76     c = confreader.File(options.configfile, is_restart=options.no_spawn)
     77 
     78     return manager.Qtile(

  /home/ei-grad/repos/qtile/bin/libqtile/confreader.py(51)__init__()
     49             try:
     50                 sys.path.insert(0, os.path.dirname(self.fname))
---> 51                 config = __import__(os.path.basename(self.fname)[:-3])
     52             except Exception, v:
     53                 # On restart, user potentially has some windows open, but they

  /home/ei-grad/.config/qtile/config.py(227)<module>()
    225 
    226 screens = [
--> 227     Screen(top=get_bar()),
    228     #Screen()
    229 ]

  /home/ei-grad/.config/qtile/config.py(222)get_bar()
    220         Metrics(font=font, fontsize=12, foreground="#A0A090"),
    221         widget.Systray(icon_size=15),
--> 222         widget.Clock(fmt="%c", font=font, foreground=foreground),
    223     ], 15)
    224 

> /home/ei-grad/repos/qtile/bin/libqtile/widget/clock.py(32)__init__()
     30         base._TextBox.__init__(self, " ", width, **config)
     31         import ipdb; ipdb.set_trace()
---> 32         self.update()
     33 
     34     def update(self):

ipdb> c
> /home/ei-grad/repos/qtile/bin/libqtile/widget/clock.py(32)__init__()
     31         import ipdb; ipdb.set_trace()
---> 32         self.update()
     33 

ipdb> where
  /home/ei-grad/repos/qtile/bin/qtile(89)<module>()
     87 if __name__ == "__main__":
     88     locale.setlocale(locale.LC_ALL, locale.getdefaultlocale())
---> 89     q = make_qtile()
     90     try:
     91         q.loop()

  /home/ei-grad/repos/qtile/bin/qtile(76)make_qtile()
     74     log = manager.init_log(log_level)
     75 
---> 76     c = confreader.File(options.configfile, is_restart=options.no_spawn)
     77 
     78     return manager.Qtile(

  /home/ei-grad/repos/qtile/bin/libqtile/confreader.py(82)__init__()
     80         # We delay importing here to avoid a circular import issue when
     81         # testing.
---> 82         from resources import default_config
     83         for option in config_options:
     84             v = getattr(default_config, option)

  /home/ei-grad/repos/qtile/bin/libqtile/resources/default_config.py(92)<module>()
     90                         widget.TextBox("default config", name="default"),
     91                         widget.Systray(),
---> 92                         widget.Clock('%Y-%m-%d %a %I:%M %p'),
     93                     ],
     94                     30,

> /home/ei-grad/repos/qtile/bin/libqtile/widget/clock.py(32)__init__()
     30         base._TextBox.__init__(self, " ", width, **config)
     31         import ipdb; ipdb.set_trace()
---> 32         self.update()
     33 
     34     def update(self):
@cjbarnes18

My understanding is that the default config is loaded so that whatever is missing from the users config is taken from the default file, so if you do not define screens object in your config, you get the default screen instance.

@ei-grad

Yes it is right, but I have screens defined in my config. I think it would be better to don't initialize unneeded values in default_config. We can initialize missing values in confreader. I can write a patch for that... Would it be acceptable solution?

@ei-grad

Or maybe the whole config logic should be refactored to use a Config class singleton object?.. Configuring qtile using a derivied Config class looks better than a config module and a default_config.py hack, imho. It could be added as alternative configuration way.

@ei-grad

Hm... no, looks like really there is no difference...

@ei-grad ei-grad added a commit to ei-grad/qtile that referenced this issue Dec 26, 2012
@ei-grad ei-grad Do not initialize unused values in default_config
Fixes #251
5f0df30
@cjbarnes18

The whole default_config.py setup is not that old (at least not the working one).

Default values for some lesser used configuration variables are currently set using the default_config.py file through libqtile.config. Removing this mechanism will mean creating a new way to set default values.

It might be worth looking to see how it was done before default_config.py

I think an alternate set of values should be set for this purpose, the default_config.py clearly should only be used where the users config is absent or broken.

Maybe the widget/layout defaults mechanism would be appropriate???

@tych0 tych0 closed this in bd79d10 Dec 26, 2012
@ei-grad

I think an alternate set of values should be set for this purpose, the default_config.py clearly should only be used where the users config is absent or broken.

And what about the way I did it in my commit 5f0df30 ?

Maybe the widget/layout defaults mechanism would be appropriate???

Do you mean defaults which are discussed in #234? Hmm, I doubt that it could be appropriate here. And as I understand it itself going to be replaced by something more accurate...

@ei-grad

@tych0 your commit doesn't fix the problem, as it is only a style change.

@tych0
Qtile member

Ah, true, python isn't a lazy language :-). I'll fix it in a sec..

@tych0 tych0 added a commit that referenced this issue Dec 26, 2012
@tych0 tych0 Really fix #251
Python doesn't have lazy semantics :-)
b7db91f
@ei-grad

Don't understand how, but it really fixes the problem in my case 0_o. Hmmm.

Oh, it now just doesn't call _configure for initialized widgets. So, with the current Clock widget implementation (not mine which runs the timer from _configure) it calls the default_config.screens[0] panel Clock widget's .update method one time per second. Plus, the another one - for the Clock widget which is on the panel which is actualy displayed.

diff --git a/libqtile/widget/clock.py b/libqtile/widget/clock.py
index 6d40593..67175d0 100644
--- a/libqtile/widget/clock.py
+++ b/libqtile/widget/clock.py
@@ -26,9 +26,11 @@ class Clock(base._TextBox):
         """
         self.fmt = fmt
         base._TextBox.__init__(self, " ", width, **config)
+        import ipdb; ipdb.set_trace()
         self.timeout_add(1, self.update)

     def update(self):
+        self.log.error("I'm called!")
         if self.configured:
             now = datetime.datetime.now().strftime(self.fmt)
             if self.text != now:

You can see there is two "I'm called!" messages per second:

ei-grad@ei-grad repos/qtile (develop *%) » DISPLAY=:1 python2 ./bin/qtile                                                                                             1> /home/ei-grad/repos/qtile/bin/libqtile/widget/clock.py(30)__init__()
     29         import ipdb; ipdb.set_trace()
---> 30         self.timeout_add(1, self.update)
     31 

ipdb> c
> /home/ei-grad/repos/qtile/bin/libqtile/widget/clock.py(30)__init__()
     29         import ipdb; ipdb.set_trace()
---> 30         self.timeout_add(1, self.update)
     31 

ipdb> c
Fontconfig warning: "/etc/fonts/infinality/conf.d/41-repl-os-linux.conf", line 16: Having multiple values in <test> isn't supported and may not work as expected
Fontconfig warning: "/etc/fonts/infinality/conf.d/41-repl-os-linux.conf", line 29: Having multiple values in <test> isn't supported and may not work as expected
Fontconfig warning: "/etc/fonts/infinality/conf.d/41-repl-os-linux.conf", line 39: Having multiple values in <test> isn't supported and may not work as expected
Fontconfig warning: "/etc/fonts/infinality/conf.d/41-repl-os-linux.conf", line 48: Having multiple values in <test> isn't supported and may not work as expected
Fontconfig warning: "/etc/fonts/infinality/conf.d/41-repl-os-linux.conf", line 60: Having multiple values in <test> isn't supported and may not work as expected
Fontconfig warning: "/etc/fonts/infinality/conf.d/41-repl-os-linux.conf", line 71: Having multiple values in <test> isn't supported and may not work as expected
Fontconfig warning: "/etc/fonts/infinality/conf.d/41-repl-os-linux.conf", line 82: Having multiple values in <test> isn't supported and may not work as expected
Fontconfig warning: "/etc/fonts/infinality/conf.d/41-repl-os-linux.conf", line 92: Having multiple values in <test> isn't supported and may not work as expected
2012-12-26 20:52:10,667 qtile update:33  I'm called!
2012-12-26 20:52:11,426 qtile update:33  I'm called!
2012-12-26 20:52:11,429 qtile update:33  I'm called!
2012-12-26 20:52:12,419 qtile update:33  I'm called!
2012-12-26 20:52:12,426 qtile update:33  I'm called!
2012-12-26 20:52:13,418 qtile update:33  I'm called!
2012-12-26 20:52:13,426 qtile update:33  I'm called!
2012-12-26 20:52:14,418 qtile update:33  I'm called!
2012-12-26 20:52:14,431 qtile update:33  I'm called!
2012-12-26 20:52:15,418 qtile update:33  I'm called!
2012-12-26 20:52:15,426 qtile update:33  I'm called!
^C---------------------------------------------------------------------------
KeyboardInterrupt                         Traceback (most recent call last)
/home/ei-grad/repos/qtile/bin/qtile in <module>()
     87 if __name__ == "__main__":
     88     locale.setlocale(locale.LC_ALL, locale.getdefaultlocale())
---> 89     q = make_qtile()
     90     try:
     91         q.loop()

/home/ei-grad/repos/qtile/bin/libqtile/manager.py in loop(self)
    561             context = gobject.main_context_default()
    562             while True:
--> 563                 if context.iteration(True):
    564                     try:
    565                         # this seems to be crucial part

KeyboardInterrupt: 

So, may be I'll make a pull request with 5f0df30 ?

@tych0
Qtile member

Ah, good point. I guess I'd like to keep the default config out of confreader.py if possible. Is there some way we can leave default_config.py mostly intact (e.g. perhaps just have a function that will tack on missing stuff in there?).

@tych0 tych0 reopened this Dec 26, 2012
@cjbarnes18

I think a slightly different approach is needed.

default_config should not be providing values for the users config. If some values need defaults (like auto_fullscreen) they should be defined in configreader not default_config.

We should not have default values for required config objects, like keys or groups...
If we do provide a default for screens it should contain one screen with no bar.

@cjbarnes18

I have applied my thinking to ei-grad's config branch in my confreader branch https://github.com/cjbarnes18/qtile/tree/confreader.

@cjbarnes18 cjbarnes18 added a commit to cjbarnes18/qtile that referenced this issue Dec 27, 2012
@ei-grad ei-grad Do not initialize unused values in default_config
Fixes #251
0054f97
@cjbarnes18

I have re-based my branch on the current develop and have all tests passing, so unless you guys have any other ideas I will send a pull request.

@tych0
Qtile member

I guess I'd like to find a way to keep the default config intact, it's nice to have a place we can point to that says "here's the defaults". Additionally, it seems odd to have default values in the config "parser" itself. Is there some way we can keep default_config.py but still initialize things lazily?

@ei-grad

I think the best solution for this will be to have an .ini-style config.

@tych0
Qtile member

Wouldn't we lose a lot of power then? Some of the more advanced configs make use of python functions at minimum. Switching config paradigms seems like making a mountain out of a molehill for this problem.

@ei-grad

No, I mean .ini config as alternative.

@cjbarnes18

ini files AFAIK can only contain strings and numbers, the current config contains python objects, to change this we would need to put another layer between the config and qtile, and in doing so we would lose a lot of the hackability in Qtile.

The problem with having default values in the default_config.py file is that to use any of the values you have to import the module which instantiates the objects within.

We could separate this out into 2 files, one for default values, the other for the fallback configuration, but I don't see how this is any better and will be prone to the same kind of problems, e.g. the current default floating layer object.

With the defaults in the parser (which as far as I have seen this is pretty common practice), we instantiate default objects only when needed.

For documentation purposes, I think we should document all config objects in the parser source and pull that out using sphinx. Happy to do this if this is how we decide to proceed.

With default values removed from default_config I think this serves as a better example of what is needed in a working config.

@ei-grad

the current config contains python objects

Actually, this is the problem. It is bad to have unused objects living with the qtile process. Ability to use .ini file for configuration (keeping the ability to use the config.py instead of .ini) is a solution. Another alternative is to remove this objects somewhere after default_config import, but it would be an ugly hack, imho.

We could separate this out into 2 files, one for default values, the other for the fallback configuration, but I don't see how this is any better and will be prone to the same kind of problems, e.g. the current default floating layer object.

As I understand, @tych0 wants to avoid this. I think this makes sense, too.

With the defaults in the parser (which as far as I have seen this is pretty common practice), we instantiate default objects only when needed.

I do not understand what kind of parser you mean and how you came to this, but yes, with the .ini file we, obviously, need some kind of parser, which would provide an interface allowing to create python objects from .ini file. And this way we could instantiate only the required objects for the config.

For documentation purposes, I think we should document all config objects in the parser source and pull that out using sphinx. Happy to do this if this is how we decide to proceed.

Not sure what do you want to do, but it looks reasonable.

@ei-grad

Other way could be to import default_config only if some config parameters are not defined in config.

@ei-grad ei-grad added a commit to ei-grad/qtile that referenced this issue Dec 30, 2012
@ei-grad ei-grad Don't import default_config unless it is needed
Another try to fix #251
ed98da7
@ei-grad

But in this case we have to force the users to define all parameters, even those which have the None/True/False values, in their configs. @tych0, is it acceptable?

@tych0
Qtile member

Isn't the problem really just that we're attaching the clock's timer in __init__ where before we were attaching them in _configure? Other widgets will exhibit the same problem, but they're not used in the default config, so it's not really an issue. The systray widget still does all of it's nontrivial configuration in _configure, which is never called if the default config is not used, so it's not an issue.

Perhaps the simplest solution is just to remove the Clock from the default config?

@cjbarnes18
@ei-grad

I think it is wrong way. It is bad that there are live unused objects in the qtile process. I'll write an .ini config implementation this week, lets see.

@cjbarnes18

I agree with you on the1st point, we should not be creating objects unless we intend to use them, it is a dirty hack way to do it.

I do think that the default_config should serve one purpose and serve it well, which should be the configuration used in the absence of a working users config.py.

I think that the config parser libqtile.confreader.File should be responsible for ensuring sane defaults are used where appropriate, this is in effect the same function that argparse does, and an ini file should be parsed in just the same way.

If you think that you can add functionality to ini as in my current config, then I will be glad to test it.

https://gist.github.com/4151805

@tych0
Qtile member
@cjbarnes18
@tych0
Qtile member

I'm going to go ahead and close this, since #185 has been merged. It's a good thing to be aware of, but I'm not sure we need to address it at this point. If it becomes a problem again, we can re-open and revisit it.

@tych0 tych0 closed this Jan 7, 2013
@horsik horsik pushed a commit that referenced this issue Jan 9, 2014
@tych0 tych0 Only initialize default config when necessary
Closes #251
b9e7b36
@horsik horsik pushed a commit that referenced this issue Jan 9, 2014
@tych0 tych0 Really fix #251
Python doesn't have lazy semantics :-)
bdca663
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment