Rotating dash generates way too much output

[egmont]

I have a directory containing 2 million files. Using script I recorded starting up mc (takes about 10 seconds) and then quitting immediately.

Without rotating dash, the resulting typescript file is around 4 kB. With rotating dash, it's about 2 MB. (This is with the default skin; with 256-color or 16M-color skins it's even more.)

mc shouldn't generate megabytes of traffic for such an eye candy. It might even cause severe usability problems over a slow network.

It's one thing that it keeps setting the foreground and background colors which could be optimized away, but it's probably not the right direction to start.

On another note, IMO the rotating dash doesn't look too nice. It spins crazily too fast.

What I have in mind:

Currently layout.c's rotate_dash() keeps the last state in a static variable, bumps it each time and draws the new shape.

Instead, the shape to be drawn should be taken from g_get_monotonic_time(), scaled to a few steps per second, modulo the number of phases to draw. Plus, the last drawn shape should be stored in a static variable and the new one only actually painted if it has changed.

This would result in a nicer look (spinning with a reasonable constant speed) and save significantly on the produced amount of data.

There are maybe one or two more places at the source where a rotating dash is presented, they should also be updated.

Let me know if you have any objection against this change. If not, I'll be happy to come up with a patch.

[egmont]

Proposal

[egmont]

Please see the attached patch. I was thinking of something like this.

Each time there was an "action", it only rotates the dash if at least 1/8 seconds have elapsed from the last time (or if the dash is initially drawn).

This arbitrarily made up constant corresponds to the theoretical upper limit of 1 complete rotation of the dash per second, which upper limit cannot be reached (would require rotate_dash() to be called again immediately once this amount of time elapses, plus rotate_dash() itself to execute immediately).

[andrew_b]

I think we should use mc_time_elapsed() instead of g_get_monotonic_time().

[andrew_b]

Replying to andrew_b:

I think we should use mc_time_elapsed() instead of g_get_monotonic_time().

mc requires glib >= 2.26, but g_get_monotonic_time is in glib >= 2.28.

[egmont]

The version requirement is a nice catch, thanks! glib-2.28 is 6.5 years old, I wouldn't mind bumping the requirement.

lib/util.c's mc_time_elapsed() uses a global timer, and as such I'm not brave enough to use it here. Even if it happens to work, it's a fragile design and we can't tell when a subsequent refactoring will make it clash with another timer. As such, I'd rather fallback to manually using its underlying mc_timer_elapsed().

However, back to mc_time_elapsed() for a moment, this is what takes care of the only tricky thing here: clock skew. In other words, makes sure we won't get stuck in the "not yet elapsed" state for a loooong time on some rare circumstances. (My patch, even though theoretically not necessary with monotonic_time, takes care of it by using unsigned ints.)

The underlying mc_timer_{new,destroy,elapsed} seems to be much ado for nothing, a truly thin wrapper that would especially be pointless if we refactored to g_get_monotonic_time().

How about the following:

I'll refactor mc so that mc_time_elapsed doesn't have the global timer hardcoded, it gets it injected as a guint64*. I'll make it rely on monotonic clock. I might add another similar convenience method as needed, it'll turn out during the actual work. I'll remove the mc_timer_t stuff. (I don't know yet how it's exactly going to look like, this is just a rough direction.) And of course make the rotating dash rely on this/these method(s) too.

I'm fine if we don't make it into 4.8.20 but only 4.8.21, then glib-2.28 will already be 7 years old.

[andrew_b]

Replying to egmont:

I'll remove the mc_timer_t stuff. (I don't know yet how it's exactly going to look like, this is just a rough direction.) And of course make the rotating dash rely on this/these method(s) too.

I have an idea to make gettimeofday(2) wrapper and simplify time compare in get_key_code(), learcn_key() and some other places and get rid of gettimeofday(2) calls at all. mc_timer_t was the first step. Other steps are partially implemented, but branch is not published and too old.

[egmont]

If we can use g_get_real_time() and g_get_monotonic_time(), which I believe we can without any worries, then I believe introducing another layer and data type (let alone the mallocs and frees) no longer makes too much sense. (It made sense of course on top of the really inconvenient gettimeofday().) Would you be up for going with these two methods and gint64 as the data type instead of mc_timer_t, and writing convenience methods on top of these?

If you insist on mc_timer_t then of course I'll rewrite my patch to go on top of that.

[andrew_b]

Replying to egmont:

Would you be up for going with these two methods and gint64 as the data type instead of mc_timer_t, and writing convenience methods on top of these?

Ok.

[andrew_b]

Branch: 3859_rotate_dash_rate
changeset:9eb2b3991ef1528fb34be665794efc3b2fcdefdb

[andrew_b]

Merged to master: [3195dd7169e4d65eb132ae7a604defdb4a7f1d9f].