window-maker / wmaker

Mirror of the official repository repo.or.cz/wmaker-crm.git. Do not send pull requests here, send your patches to wmaker-dev@googlegroups.com instead
https://repo.or.cz/wmaker-crm.git
GNU General Public License v2.0
136 stars 18 forks source link

Fix #5 (slow workspace switching) when wsmap is unused. #6

Closed cme closed 3 years ago

cme commented 3 years ago

Partial fix for issue #5 which skips updating workspace map preview images on workspace switching, until the first time the workspace map is shown.

This covers use cases (including default Debian configuration) where the workspace map is never used.

Also removes a redundant call to wWorkspaceMapUpdate() on starting the workspace map (which was doubling the time taken for the map to appear when it's actually used).

crmafra commented 3 years ago

On Sun, 13 Dec 2020 at 4:04:39 -0800, Colin McEwan wrote:

Partial fix for issue #5 which skips updating workspace map preview images on workspace switching, until the first time the workspace map is shown.

This covers use cases (including default Debian configuration) where the workspace map is never used. You can view, comment on, or merge this pull request online at:

https://github.com/window-maker/wmaker/pull/6

-- Commit Summary --

  • Fix #5 (slow workspace switching) when wsmap is unused.

-- File Changes --

M src/wsmap.c (17)

-- Patch Links --

https://github.com/window-maker/wmaker/pull/6.patch https://github.com/window-maker/wmaker/pull/6.diff

Wouldn't it be enough to unset the "Enable workspace pager" option in the Expert panel of WPrefs?

cme commented 3 years ago

Wouldn't it be enough to unset the "Enable workspace pager" option in the Expert panel of WPrefs?

Do you mean, to disable that by default, or that a user experiencing slow workspace switching should disable it in WPrefs?

I see issues with either of those:

  1. disabling by default means the user would have to take two steps to use the workspace map: currently they would have to bind a key to it, disabling by default they'd also have to re-enabled it (or have WPrefs manage that dependency and enable it for them)
    1. there's no way for a user to know that it's the workspace map that's causing this slow switching, and therefore connect "workspace switching is awfully slow" with "I should disable a feature that I'm not using, and may not even have heard of."

Making the preview update somewhat lazy like this has the other advantage that the overhead of creating the preview also occurs when bringing up the workspace map, so the user stands a change of connecting the lag with that feature. I suppose an alternative solution might be to update the preview only when switching workspaces using the workspace map, and not when switching using the clip or keyboard.

crmafra commented 3 years ago

On Sun, 13 Dec 2020 at 5:16:41 -0800, Colin McEwan wrote:

Wouldn't it be enough to unset the "Enable workspace pager" option in the Expert panel of WPrefs?

Do you mean, to disable that by default, or that a user experiencing slow workspace switching should disable it in WPrefs?

The workspace pager is already disabled by default. And iirc, it was disabled by default because of this slow switching.

I see issues with either of those:

  1. disabling by default means the user would have to take two steps to use the workspace map: currently they would have to bind a key to it, disabling by default they'd also have to re-enabled it (or have WPrefs manage that dependency and enable it for them)

  2. there's no way for a user to know that it's the workspace map that's causing this slow switching, and therefore connect "workspace switching is awfully slow" with "I should disable a feature that I'm not using, and may not even have heard of."

I am not sure I agree with this. The feature is disabled by default and wmaker is fast by default.

If you go to the Expert panel of WPrefs and enable it and wmaker becomes slow after that, you'll know the culprit.

Making the preview update somewhat lazy like this has the other advantage that the overhead of creating the preview also occurs when bringing up the workspace map, so the user stands a change of connecting the lag with that feature. I suppose an alternative solution might be to update the preview only when switching workspaces using the workspace map, and not when switching using the clip or keyboard.

Maybe I am misunderstanding your patch.

From your description you seem to not be using the workspace map because the keyboard shortcut was not set. Yet you were still having a slow workspace switching experience.

So my impression is that your patch addresses the slow switching but you still continue to not use the workspace map feature (ie you have no keyboard shortcut for it). My suggestion to disable the feature was based on this interpretation.

But now I think that your patch is solving a bigger issue: when you choose to use the workspace map (ie you have a shortcut for it and you enable it on WPrefs) you'll get a slow workspace map only when you hit the keyboard shortcut and not everytime you switch workspaces. Is that what your patch is meant to do?

cme commented 3 years ago

The workspace pager is already disabled by default. And iirc, it was disabled by default because of this slow switching.

Aaaaaha! This is what I have been missing. It looks like sometime in the dim and distant past I had (by accident or by deadly curiosity) managed to enable it. I wouldn't have noticed the slowdown in workspace switching at the time, because I was using a lower resolution and the time taken seems to be non-linear.

So because those events were separated by some amount of time, I didn't encounter the connection and assumed that Enabled was the default behaviour.

Excellent. I am now entirely happy. Thank you! :)

But now I think that your patch is solving a bigger issue: when you choose to use the workspace map (ie you have a shortcut for it and you enable it on WPrefs) you'll get a slow workspace map only when you hit the keyboard shortcut and not everytime you switch workspaces. Is that what your patch is meant to do?

No, your initial interpretation was correct.

I think the behaviour of only updating (and potentially hitting the slow condition) when using the pager would be desirable though in cases where the pager is enabled and used, which is an even simpler change than this one, but could lead to stale and misleading previews.