[openbox] "conditional appearance for titles" -- request for Code Review

[Nitzan Shaked]

Thanks again for your reply.

I am not sure I understand why you are suggesting putting the strings in
rc.xml. You are referring to the strings "machine1.mycompany.com" etc,
correct? If so, and especially since the result of a match is an
_appearance_ change, I think the themerc file is the place. I may have
misunderstood you, though.

Your suggestion regarding using a per-property for the appearance is nice
indeed. However, the following comes to mind:
1) I am not sure whether a window's name or class can change. My "feature"
works also when the title changes in run time (after the window has been
created), that's (part of) the point.
2) I am not sure that when they do change, the WM easily knows about that.
(Again: for me to go and look it up...)
3) The current code can parse "appearances" (bg, color, colorTo, etc) from a
themerc file, and not from an XML file. That is also one of the reasons I
implemented my "feature" the way it is now.

I am allowing myself to interpret your suggestion differently: you are
actually suggesting to use the "dos regexp" to match the per-app settings.
This should be doable, and if easy enough I'll do it right now.

Again, I welcome comments, especially regarding (1)..(3) above.

Nitzan

-----Original Message-----
From: Javeed Shaikh [mailto:syscrash2k at gmail.com] 
Sent: Saturday, October 21, 2006 00:05 AM
To: openbox at icculus.org
Subject: Re: [openbox] "conditional appearance for titles" -- request for
Code Review

The correct place to put the condition text is probably rc.xml.

Openbox currently has a feature that allows you to set settings for windows
based on their WM class and name (you can use wmctrl to get these values for
all open windows.) For example, you can set something to make a certain
window always appear on all desktops. Your condition text idea is probably
best if adapted to the current system.
The following, for example, makes all epiphany windows open at a specific
position:

<application class="Epiphany">
  <position>
    <x>60</x>
    <y>60</y>
  </position>
</application>

(you can try it in rc.xml if you wish!)

If you adapted your title colour changing to this system, you could have
something like <application class="Epiphany">
  <position>
    <x>60</x>
    <y>60</y>
  </position>
  <titleColour>#ff0000</titleColour>
</application>

The name of the property (titleColour) is just one thing that came to mind
(I'm not very good with naming these things.)

I think your patch is more likely to be included in openbox if you make your
title colour changing work with the existing system.

In my original reply, when I mentioned per-app settings, I was trying to say
that your regexp stuff could probably extend the current system of using the
class property of a window. Perhaps you could make it possible to match a
window by name.
If I recall
correctly, there is already a comment in the code (in the function where
per-app matching
occurs) that says something like 'this should probably be more fancy.'

Enjoy!

On 10/20/06, Nitzan Shaked <calius at netvision.net.il> wrote:
> Thank you.
>
> Where you think is the correct place to put the condition text?
>
> I am not sure I understand your per-app matching idea. Can you 
> elaborate please?
>
> Cheers,
> Nitzan
>
> -----Original Message-----
> From: Javeed Shaikh [mailto:syscrash2k at gmail.com]
> Sent: Friday, October 20, 2006 23:50 PM
> To: openbox at icculus.org
> Subject: Re: [openbox] "conditional appearance for titles" -- request 
> for Code Review
>
> Hello,
>
> That sounds like an interesting idea. However, from what I can see, 
> the condition text has to be set in the theme. Perhaps you should change
that.
>
> Another interesting thing is your mini regexp matcher. It would 
> probably be cool if you made that also work for per-app settings.
>
> Mikachu (the maintainer) probably has some suggestions too.
>
> Not bad work though!
>
> On 10/20/06, Nitzan Shaked <calius at netvision.net.il> wrote:
> > Hello all
> >
> > I've implemented a small feature: conditional appearance for the 
> > "focused title", based on the current title text. I use it to change 
> > the bg color of my xterm titles based on which machine I am logged 
> > on to, and which user I am. I added
> >
> > alias precmd 'echo -n "\033]0;${HOST} : `whoami`\007"'
> >
> > To all users' .tcshrc, and thus the xterm title changes whenver I 
> > ssh to another machine, and/or whenever I "su". The new feature then 
> > changes the color of my xterm's title.
> >
> > I am attaching 3 files: a diff file (svn diff from an SVN checkout 
> > done 3hrs
> > ago) and 2 new files to be placed under "openbox/".
> >
> > I am requesting:
> > 1) a code review from the maintainer / someone in charge, if this is 
> > interesting to anyone.
> > 2) your endless patience and understanding :) it's the first time I 
> > am ever submitting code to a public project.
> >
> > I tried following code convention, and writing something relatively
> modular.
> > I hope I did okay.
> >
> > Comments anyone?
> >
> > Cheers,
> > Nitzan
> >
> >
> >
>
> __________ NOD32 1.1454 (20060321) Information __________
>
> This message was checked by NOD32 antivirus system.
> http://www.eset.com
>
>
>

__________ NOD32 1.1454 (20060321) Information __________

This message was checked by NOD32 antivirus system.
http://www.eset.com