Subject: Style

From: Olivier Chapuis <[email-masked]>
Date: Sat, 12 Feb 2000 14:33:56 +0100

Hello,
Now that we have dynamic styles, it is natural that "theme engine"
process a lot of styles without any restart. The problems is that
this increase the memory usage, since the Style command add and
add and add style ...

I suggest that we change  add_style_to_list as follow:

static void add_style_to_list(window_style *new_style)
{
  /* This used to contain logic that returned if the style didn't contain
   * anything.  I don't see why we should bother. dje.
   *
   * used to merge duplicate entries */

  window_style *temp = all_styles;
  Bool add = 1;

  if(last_style_in_list != NULL)
  {
    /* not first entry in list chain this entry to the list */
    while (temp)
    {
      if (matchWildcards(SGET_NAME(*new_style), SGET_NAME(*temp)) == TRUE)
      {      
	merge_styles(temp, new_style, True);
	add = 0;
      }
      temp = SGET_NEXT_STYLE(*temp);
    }
    if (add) 
    {
      SSET_NEXT_STYLE(*last_style_in_list, new_style);
      last_style_in_list = new_style;
    } 
    else 
    {
      free_style(new_style);
      free(new_style);
    }
  }
  else
  {
    /* first entry in list set the list root pointer. */
    all_styles = new_style;
    last_style_in_list = new_style;
  }

  return;
} /* end function */

I think that it is clear that this gives the same result (however,
I've not wrote a proof of that). The main advantage is that with
this the style list do not go to infinity (and we save memory).

Ok, we can use DestroyStyle, but the problem with destroy style is
that it is not easy to use to destroy a lot of styles. So, if "we"
do not want to change add_style_to_list, I think that we need a
DestroyMatchingStyle command.

Any opinion ?

Olivier
--
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: Mikhael Goikhman <[email-masked]>
Date: Sat, 12 Feb 2000 18:41:34 +0200

> I suggest that we change  add_style_to_list as follow:

Seems like a good memory fix, it is better that Dominik verified this too.
A minor comment correction:

-     * used to merge duplicate entries */
+     * Merges duplicate entries. */

Still I think we need something like DestroyMatchingStyles sooner or
later.

Regards,
Mikhael.

---

From: Dominik Vogt <[email-masked]>
Date: Mon, 14 Feb 2000 09:45:44 +0100

> static void add_style_to_list(window_style *new_style)

...

Can you please provide a unified diff?  It makes reading much
easier and faster.  I just don't have the time to do it myself.

Bye

Dominik ^_^

-- 
Dominik Vogt, Agilent Technologies, Dept. BVS
Herrenberger Str.130, 71034 Boeblingen, Germany
phone: 07031/464-4596, fax: 07031/464-3883, [email-masked]
--
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, 14 Feb 2000 11:17:41 +0100

> easier and faster.  I just don't have the time to do it myself.

Here it is, Olivier

--- fvwm/style.c.orig   Fri Feb 11 14:20:26 2000
+++ fvwm/style.c        Mon Feb 14 11:15:46 2000
@@ -421,22 +421,40 @@
   /* This used to contain logic that returned if the style didn't contain
    * anything.  I don't see why we should bother. dje.
    *
-   * used to merge duplicate entries, but that is no longer
-   * appropriate since conflicting styles are possible, and the
-   * last match should win! */
+   * merge duplicate entries */
+
+  window_style *temp = all_styles;
+  Bool add = 1;
   if(last_style_in_list != NULL)
   {
     /* not first entry in list chain this entry to the list */
-    SSET_NEXT_STYLE(*last_style_in_list, new_style);
+    while (temp)
+    {
+      if (matchWildcards(SGET_NAME(*new_style), SGET_NAME(*temp)) == TRUE)
+      {
+       merge_styles(temp, new_style, True);
+       add = 0;
+      }
+      temp = SGET_NEXT_STYLE(*temp);
+    }
+    if (add)
+    {
+      SSET_NEXT_STYLE(*last_style_in_list, new_style);
+      last_style_in_list = new_style;
+    }
+    else
+    {
+      free_style(new_style);
+      free(new_style);
+    }
   }
   else
   {
     /* first entry in list set the list root pointer. */
     all_styles = new_style;
+    last_style_in_list = new_style;
   }
-  last_style_in_list = new_style;
-  merge_style_list();
   return;
 } /* end function */
--
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: Dominik Vogt <[email-masked]>
Date: Mon, 14 Feb 2000 11:42:55 +0100

> > easier and faster.  I just don't have the time to do it myself.

Hm, the new part is matching against patterns instead of
the literal style name.  The style list is already merged
if possible (i.e. two styles with the same name are merged
if no other style is in between).  Handling wildcards may
have unwanted effects with DestroyStyle:

 1) Style xterm foo
 2) Style * bar ==> styles will be merged as if
    you issued 'Style xterm foo, bar'
 3) DestroyStyle * ==> You'll end up with 'bar' as a
    style for xterms.

I'm not sure if this is acceptable.  Perhaps the merging
logic should be more aggressive.  For example

 Style foo StippledTitle, BorderWidth 4
 Style bar SomeStyle
 Style foo StippledTitle
 Style foo BorderWidth 6

Currently this would be represented in the style list by

 foo: StippledTitle, BorderWidth 4
 bar: SomeStyle
 foo: StippledTitle, BorderWidth 6

But since the first 'foo' style is completely overridden
by the second 'foo', it can be deleted.

Bye

Dominik ^_^

---

From: Olivier Chapuis <[email-masked]>
Date: Mon, 14 Feb 2000 22:13:03 +0100

>  3) DestroyStyle * ==> You'll end up with 'bar' as a

Yep, so if we just replace 

by

if (strcmp(SGET_NAME(*new_style), SGET_NAME(*temp)) == 0)

I think that every things will be ok

> by the second 'foo', it can be deleted.

This is done by the patch :)

Olivier

---

From: Dan Espen <[email-masked]>
Date: Mon, 14 Feb 2000 16:38:04 -0500

...
> DestroyMatchingStyle command.

Aggh!  I hate looking at the style merging code, it never fails
to confuse.

I don't follow what the patch is intended to do, but I don't see
how it can combine style entries with any entry other than the
one previous, and only when it has the same name.

This is the due to the way they are merged from the beginning
to the end when a window is created.  The only old ones that
could be removed are ones that set the same attributes.

More important, I believe the theme engine would mostly be changing
style "*".  In fact, a large majority of style commands are for
style "*".  Same style entries are always merged.

--
Dan Espen
444 Hoes Lane  Room RRC 1C-214           E-mail: [email-masked]
Piscataway, NJ 08854                     Phone: (732) 699-5570
--
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: Mikhael Goikhman <[email-masked]>
Date: Tue, 15 Feb 2000 00:20:06 +0200

> one previous, and only when it has the same name.

It is possible if you merge the new style entry with all previously
matching ones instead of adding it, i.e. you add foo from Style "mywin*"
to previousely defined Style "my*".

This would be ok if not Dominik's reason - DestroyStyle will be broken.

> style "*".  Same style entries are always merged.

Only if there are no other style entries in between.

What if a user supplies a list of name paterns to use another style with?
I.e. there are several window styles used. This is more difficult to
implement for the theme engine, but theoretically possible.

Also there are dosens of lines like: Style "XTerm" Icon, MiniIcon.

And of course it is just a plain bug when you reload the same file with
Style "bla-bla" lines and have the memory duplicated.

Regards,
Mikhael.

---

From: Mikhael Goikhman <[email-masked]>
Date: Tue, 15 Feb 2000 00:50:28 +0200

> I think that every things will be ok

No, this will break:
   Style "my*" foo
   Style "mywin*" bar
   Style "my*" bar

> > by the second 'foo', it can be deleted.

So instead of merging and not adding, the right thing is to always add,
but then remove all previous subsets of this entry (check flag_mask).

  foo: StippledTitle
  foo: BorderWidth 4, NoHandles
  bar: SomeStyle
  foo: StippledTitle, BorderWidth 6

The first entry is removed.
Dominik, did you plan to work on this solution? If not, I or Olivier
will prepare another patch.

Regards,
Mikhael.

---

From: Dan Espen <[email-masked]>
Date: Mon, 14 Feb 2000 20:28:40 -0500

> will prepare another patch.

The example is bad, you can't have 2 foo's in a row. I get the point
though.

I still think any repetitively issued style commands
are going to be for style "*".

There are a load of style arguments to check so detecting
subsets is non-trivial.  Its not just looking at the flag_mask.
I could be wrong.

For most users, the additional code will use more memory than the
few extra styles.  (Sooner or later you're going to get the styles
the way you like.)

Doesn't look like bug fixing to me.

---

From: Mikhael Goikhman <[email-masked]>
Date: Tue, 15 Feb 2000 06:05:14 +0200

> are going to be for style "*".

Here is the iconstyles file (real life example):
	Style "*"     Icon .., MiniIcon ..
	Style "XTerm" Icon .., MiniIcon ..
	Style "Gimp"  Icon .., MiniIcon ..
	... another 239 lines with Icon, MiniIcon ...

One can use individual Font, WindowListSkip, NoHandles or others as well.

> subsets is non-trivial.  Its not just looking at the flag_mask.

I hope flag_mask bits were designed for this purpose.

> few extra styles.  (Sooner or later you're going to get the styles

Most users do not use a half of the options fvwm provides, does this mean
all these options should be removed? :)

> Doesn't look like bug fixing to me.

If the iconstyles file above is reloaded, new 239 entries (~50Kb) are
allocated and the first ones are never used; then you reload it again...
The memory leak. And it is not the only memory leak of this type in fvwm.

Note, I will not argue here the needness of (re)loading configurations.
We can discuss dynamical configurations on the fvwm-themes mailing list.

Regards,
Mikhael.

---

From: Dominik Vogt <[email-masked]>
Date: Tue, 15 Feb 2000 09:46:42 +0100

Sorry if I don't reply to all the recent mails in this thread
individually and don't quote the relating portions...

The style merging stuff is designed to be backwards compatible.
That's the reason why only adjacent styles in the list are
merged.  I'd like to sacrifice compatibility in 3.0 (together
with the ...Style commands).

Yes, the flag_mask bit of styles is designed to identify
the styles that are set in a style structure.  *Each* style
has a flag, so an algorithm to remove subsets of later styles
could be:

  1) Let n be the name of a style
  2) s := last style with name n in the list
  3) f := flag_mask(s)
  4) if (no style with name n before s in the list exists): finished
  5) t := last style with name n in list before s
  6) if ((flag_mask(t) & f) == flag_mask(t)): delete style t; else: s := t
  7) f |= flag_mask(t)
  8) goto 4

All unnecessary styles can be deleted in a single pass through
the list.

But I don't think that algorithm will help much.  I wouldn't
write the code for this (I volunteer to write it) unless it's
very important for 2.4.

Bye

Dominik ^_^

---

From: Mikhael Goikhman <[email-masked]>
Date: Tue, 15 Feb 2000 15:44:45 +0200

> write the code for this (I volunteer to write it) unless it's

Ok, I will let you decide how important is it. For fvwm-themes developers
not fixing these problems means that themes can't be effectivelly switched
on fly until 3.0 without a Restart (which is very resource hangry).

Regards,
Mikhael.

---

From: Dominik Vogt <[email-masked]>
Date: Tue, 15 Feb 2000 14:47:39 +0100

On Tue, Feb 15, 2000 at 03:44:45PM +0200, Mikhael Goikhman wrote:
> Ok, I will let you decide how important is it.

No, I don't want to decide it.  My knowledge of your themes setup
is too limited.

Bye

Dominik ^_^

---

From: Dan Espen <[email-masked]>
Date: Tue, 15 Feb 2000 09:17:40 -0500

> No, I don't want to decide it.  My knowledge of your themes setup

I don't want my comments to be taken as "against" this change either.

I see that there do seem to be flags for all the style features.
I haven't looked at this code in a while.

I would have guessed that icon sets would change by changing the
imagepath.  If they change by issuing new sytle statements, and the
theme package has that many icons defined, that would use up some memory.

Is there some place where I could get more information on this theme
package?

---

From: Dominik Vogt <[email-masked]>
Date: Tue, 15 Feb 2000 16:04:21 +0100

> I haven't looked at this code in a while.

Perhaps it's time to explain some of the changes I made in the
last half year.

The 'Style w/o Recapture' patches required a change flag for
every style, not just the flag styles.  Since then there are
three flag structures:

 - the style flags themselves (some of them are unused, e.g.
   the BorderWidth flag).
 - the flag_mask that indicates if the style is defined
 - the change_mask that indicates the styles that have changed
   since the last updates were applied

When a style is added, it is merged with all previous style
definitions with the same name as long as no other style with
a different name occurs in between.

At the end of style.c there is a function check_window_style_change()
that translates the changed style flags into measures that have
to be taken to update a window with this style, e.g. if the
has_border_width flag has changed, the window has to be redecorated.

This function is called by flush_window_updates() in update.c
which in turn is called when a) a style has changed and b) fvwm
is idle.  flush_window_updates() takes care of updating all
windows and global settings like root key bindings.

Bye

Dominik ^_^

---

From: Mikhael Goikhman <[email-masked]>
Date: Wed, 16 Feb 2000 22:50:12 +0200

> theme package has that many icons defined, that would use up some memory.

ImagePath is a constant, changing it does not help anyway.
To affect Icon and MiniIcon style options of already existing windows
you should reread the iconstyles file mentioned in this thread.
There is no way to update icons in existing menus without recreating them.

Another similar problem here is that if you want to dynamically disable
all desktop icons, it is enough to command Style "*" NoIcon, but if you
want to enable all icons after that, you should reread iconstyles again,
because there is no something similar to: RemoveStyleOption "*" NoIcon.

And icons are not the biggest problem at all, you can join the
fvwm-themes-devel mailing list, where we discuss all problems.

> Is there some place where I could get more information on this theme

Yes, sure. :)

If we are speaking about this, please don't make conclusions from the
first public release. We already do everything to unload all unused memory
and will do even more when it will be implemented "apply only minimal
needed changes" feature. Still, although there will be less memory lost
than now on every change, it will be greater than 0 without fvwm changes.

Regards,
Mikhael.

---

From: Dan Espen <[email-masked]>
Date: Tue, 22 Feb 2000 11:14:23 -0500

> because there is no something similar to: RemoveStyleOption "*" NoIcon.

I think I'm convinced about the need for more powerful style merging.
I just want to comment on removing the "noicon" style.

This almost works:

Style * NoIcon
Style * Icon

the problem is that fvwm forgets about all the icons associated with styles
except for "Style * unknown.xpm".  Application supplied icons do come back.

I'm not positive, but I think this is a bug.

---

From: Dominik Vogt <[email-masked]>
Date: Wed, 23 Feb 2000 20:23:58 +0100

> I think I'm convinced about the need for more powerful style merging.

Okay.  I've coded a more aggressive algorithm (but didn't test it well).

> I'm not positive, but I think this is a bug.

Hm.  You can't have both with the same command.  On one
hand you want that 'Icon' w/o parameters enables icons
without enabling the pictures, on you other hand 'Icon'
should simply negate 'NoIcon' without resetting icons.
I don't think this can be resolved without breaking
existing config files.

Bye

Dominik ^_^