Subject: StyleById patch voting thread

From: Dominik Vogt <[email-masked]>
Date: Mon, 19 May 2003 13:44:27 +0200

Should the StyleById patch be applied before 2.6?  Please cast
your votes here.

I vote for "no" because

  1. It will even further delay delay the 2.6 release.
  2. It will destabilise the code because of the delicate nature of
     the style to window propagation.
  3. It does not introduce a data type identifying a style, just
     passes lists of arguments to the style functions.
  4. It unnecessarily exports knowledge of style related design
     choices to sub functions that do not need to know about them.
  5. Because id-specific styles are kept in the same list as the
     normal styles, it will cause memory consumption by the style
     list to explode under certain situations.  Clean rules for a
     new style preference order are needed first.
  6. It introduces inacceptably complicated if-conditions and code
     duplication.
  7. We will regret the ad-hoc syntax very soon:
      - It is not flexible (mixing different kinds of style patterns
        is difficult).
      - The syntax does not help with state-specific styles.
      - The syntax does not help with regexp style name matching.
      - It may further complicate abstracting the style parsing
        code to allow all style names in other contexts (e.g.
        conditions).

Bye

Dominik ^_^  ^_^
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Olivier Chapuis <[email-masked]>
Date: Mon, 19 May 2003 18:59:52 +0200

> Should the StyleById patch be applied before 2.6?  Please cast

I vote "yes". But maybe the patch should be applied later.

I recall my intention: style by id is a great feature, with a simple
_hack_ we can get it, so it is difficult to me to do not wrote such
code.  Yes it is a "hack" it is not the new great "WindowStyle"
command we want (which needs more discussion and an enormous code
reworte).  Yes it is just a simple piece of code with some theoretical
problems, but ok I think in practice.

About the code: a style in the style list is identified by its name.
With the StyleById patch a style in the style list is identified with
either by the XID used (generated by the StyleById command) or the name
(generated by the Style command) not both. The only change in the
earth of the style code is the test "these two styles have the same
"identifier"". The rest of the code is basically obvious ("decoration"):
parse and use already written function. 

>      the style to window propagation.

About this two it is difficult to be sure of something. I think that
the patch will cause only very few problems. If we want to do a
"perfect" WindowStyle command, then this surely delay a lot 2.6 and
destabilize the code. But StyleById is not perfect by nature. 

>      passes lists of arguments to the style functions.

Not sure to understand. There is a new element in the window_style
structure xid (!=0 if and only if the window_style was generated
by the StyleById command).

>      choices to sub functions that do not need to know about them.

Not sure to understand. As FocusStyle vs Style? Not a big issue
IMHO.

>      new style preference order are needed first.

Adding _a_ style by id is the same thing as adding a style with a name
which do not already appear in the list. There is style list
simplification/merging in between style by id. Only UseStyle is not
supported by the patch (not a big issue, this can be documented).

>   6. It introduces inacceptably complicated if-conditions and code

The complicate if-condition is just a visual complication. We can
add a macro STYLES_HAS_THE_SAME_IDENTIFIERS(*style1, *style2) and
maybe a macro which test if a fw window "match" a style "identifier".
I do not see code duplication, but the way to safely do a
strtoul (we can put this in one function somewhere). 
 
>         code to allow all style names in other contexts (e.g.

As I said It is just a tmp command. 

Regards, Olivier

---

From: Dan Espen <[email-masked]>
Date: Mon, 19 May 2003 13:24:51 -0400

> As I said It is just a tmp command. 

If its just a temporary command, it would make sense to wait until
after 2.6 although I'm not sure why a temporary command
should ever be committed.

-- 
Dan Espen                           E-mail: [email-masked]

---

From: Olivier Chapuis <[email-masked]>
Date: Mon, 19 May 2003 20:47:13 +0200

Regards, Olivier

---

From: Mikhael Goikhman <[email-masked]>
Date: Tue, 20 May 2003 02:01:55 +0000

> problems, but ok I think in practice.

How exactly "StyleById <id>" is different from "WindowStyle"?
Both implement exactly the same thing: individual window styles.
I see no reason to have a temporary syntax when there is a final syntax
that we like even if the current implementation may be not the best one.

Regarding some Dominik concerns.

First, probably the current Style and DestroyStyle implementation is
delicate and even problematic. But since the new command uses only the
existing implementation, it does not adds problems by itself. It may only
help to find the _existing_ problems faster since it does DestroyStyle
more often than usually.

So, I would say the new command may only help to stabilize Style code if
it is not stable yet (seems pretty stable). The new code is really small
to add any instability by itself, I will test it throughly anyway.

Second, I can understand some Dominik worries regarding the code purity,
but if we are honest, a possibility to have individual window styles in
this year (i.e. 2.6.0) is much more tempting than minor code issues.
It is not that the code is out of control now (I can understand what the
patch adds without looking at the rest of the code).

I thought about weither individual window style entries (one per window)
could be stored in a separate list and never be merged (only deleted),
i.e they always take a precedence. But this seems to be non intuitive,
probably a user wants all terminals to be affected by "Style XTerm
Borders" even the ones that were individually defined to be borderless,
so the logics in the patch (merging both Style types together) is good.

And, BTW, my vote is "yes", but the command name should be "WindowStyle".
And it should work on the window context, a trivial change to the patch.

Regards,
Mikhael.

---

From: Dan Espen <[email-masked]>
Date: Mon, 19 May 2003 22:22:47 -0400

> And it should work on the window context, a trivial change to the patch.

The patch does look pretty simple.
The patch lacks documentation and test cases.

---

From: Dan Espen <[email-masked]>
Date: Mon, 19 May 2003 22:38:30 -0400

> And, BTW, my vote is "yes", but the command name should be "WindowStyle".

Maybe I'm missing something.
I never completely understood all the ins and outs of the windowstyle
proposal.  Why isn't this:

Style [ id=$[w.id] ] ....

---

From: Dominik Vogt <[email-masked]>
Date: Tue, 20 May 2003 08:32:03 +0200

Some further explanations below.

> by the StyleById command).

When style identifiers become more complex than a simple string,
they need to have their own data type, e.g.

  typedef struct
  {
          char *name;
          XID window_id;
          struct
          {
                  has_name;
                  has_window_id;
          } flags;
  } style_id_t;

> Not sure to understand. As FocusStyle vs Style? Not a big issue

See above.  Passing all the possible style identifiers through the
function interface to functions parsing some of the styles is not
a good idea.

> supported by the patch (not a big issue, this can be documented).

You need to understand the problem the style list already has.  If
you issue style commands in the wrong order, the list grows ad
infinitum because styles can not be merged or deleted.  Configs
with many style changes (theme switching!) or styles tied to a
window id increase this probability a lot because they add
incompatible style identifiers to the list that prevent merging.

The reson for this is the notion that the last style always wins.
Yes, this is user friendly, but it is difficult to manage in the
code, and introduces a kind of memory leak.  I'd prefer a
hierarchical style preference system

> maybe a macro which test if a fw window "match" a style "identifier".

Ack, please no code in macros!  That's what functions were
invented for :-)

> As I said It is just a tmp command. 

Then I agree with Dan:  I see no reason to introduce temporary
commands into stable releases.

Bye

Dominik ^_^  ^_^

---

From: Mikhael Goikhman <[email-masked]>
Date: Tue, 20 May 2003 09:26:23 +0000

> Style [ id=$[w.id] ] ....

This is the syntax I prefered some time ago too, actually:

  Style (Id $[w.id]) ...

But now I think it is relatively bad compared to simply "WindowStyle ...",
because it allows to pass incorrect parameters (unexisting window ids)
and because I think it is ugly and redundant for commands and functions
that by definition should work on the window context to receive window id
as a parameter.

Think about "WindowStyle Sticky" as equivalent of "Stick", both should
work on the window context, it is just intuitive.

Regards,
Mikhael.

---

From: Olivier Chapuis <[email-masked]>
Date: Tue, 20 May 2003 14:55:08 +0200

> so the logics in the patch (merging both Style types together) is good.

In fact I am not sure. Take this example: a user want a window with
NeverFocus (an xterm as he has executed less -f .xerror and maybe
later he will want to get SloppyFocus again on it if he quit less).
So he set NeverFocus on this window (by id) ... then he reload some themes
component which leads to "Style * SloopyFocus" ... the log xterm
has SloppyFocus. Bad.

The two approach seem reasonable.

Regards, Olivier

---

From: Olivier Chapuis <[email-masked]>
Date: Tue, 20 May 2003 15:13:27 +0200

> incompatible style identifiers to the list that prevent merging.

I've wrote a "Styles" subject to the PrintInfo cmd which count the
styles in the style list and print the style names (I can commit this
if you want). With fvwm-themes theme switching does not increase the
number of styles! This is probably because fvwm-themes is well written
... and __simplify_style do a very good job!

In fact, it seems to me that you should have to be careful (and do
strange things) for making the style list growing and growing (say
with two style name).

Anyway, maybe style by id should be prioritary vs style by name:
apply style by name and then apply style by id?

> commands into stable releases.

If we add this command and in the future re-design the styles cmds,
it will be surely possible to warp style by id to one of the new
command?

Regards, Olivier

---

From: Olivier Chapuis <[email-masked]>
Date: Tue, 20 May 2003 11:06:10 +0200

> How exactly "StyleById <id>" is different from "WindowStyle"?

Yes the <id> param is not useful. But, I am just afraid that the
future WindowStyle has a syntax which is not compatible.  So, as
WindowStyle is reserved I will prefer an other name as StylePerWindow.

Regards, Olivier

---

From: Mikhael Goikhman <[email-masked]>
Date: Tue, 20 May 2003 19:05:15 +0000

> WindowStyle is reserved I will prefer an other name as StylePerWindow.

If WindowStyle is reserved, it has exactly the same syntax, just because
it should do exactly the same thing. Otherwise, it is not reserved.

I just like "WindowStyle" name more than StyleById or StylePerWindow. :)

Regards,
Mikhael.

---

From: Mikhael Goikhman <[email-masked]>
Date: Tue, 20 May 2003 19:21:45 +0000

> The two approach seem reasonable.

Yes, but then I predict the following scenario may happen too often.
Someone is surprised that some window does not accept focus and Style or
FocusStyle do not work on it, he tries several possibilities with all
names shown by FvwmIdent, then he gives up, asks on the mailing list and
we all debug this situation. At the end it happens that the problematic
window was set NeverFocus individually several months ago and the only
way now to undo this is to execute "WindowStyle SloppyFocus" on it.

Another argument for the "total merging" approach is that "theme
switching" should not be different than Restart, and on Restart
all individual window styles are lost (at least now).

Finally, if in the future all state commands are replaced by WindowStyle,
I would think this implies the first approach, at least by default.

I agree that two approaches seem reasonable, but I think the first is
more intuitive (even if it is bad in some situations).

It is possible to give an argument to note the second approach like:

  WindowStyle Permanent Stick

This may be done in the future (adding a boolean to a style identifier).
For now I would leave the functionality as in the patch.

Regards,
Mikhael.

---

From: Dan Espen <[email-masked]>
Date: Tue, 20 May 2003 21:19:07 -0400

> work on the window context, it is just intuitive.

I guess.

It could also be:

Style [] ...

I think econimizing our commands makes some sense.
Otherwise, explaining the command will be a bit clumsy.

Does this make sense:

WindowStyle:

This is just like the Style command and takes the same options,
except it applies to the "context" window.

On the other hand:

Style [xxxx]

The xxxx matches a window on name,class, or resource.  If omitted,
it matches the "context" window.

So, do you think it makes more sense if the id is omitted?