sdias / win-10-virtual-desktop-enhancer

An application that enhances the Windows 10 multiple desktops feature by adding additional keyboard shortcuts and support for multiple wallpapers.
MIT License
1.77k stars 269 forks source link

Added a function to change desktop names "On the run". #66

Closed GioBonvi closed 7 years ago

GioBonvi commented 7 years ago

Feature proposed by @achim-t in #60.

Added a new customizable shortcut (and updated documnetation accordingly) to show a popup which lets the user dinamically change the name of the current desktop.
The change is not permanent and will be overwritten by the default values in 'settings.ini' when the program will be reloaded.

sdias commented 7 years ago

The code looks good apart from a few small things that I noted. I tried it out though, and it doesn't seem to be working correctly all the time, although I'm 99% percent sure it isn't because of these changes but instead due to a pre-existing bug. What I am seeing is that if I switch back and forth between desktops, some of which are named like this, sometimes the default name appears instead (i.e. Desktop N). I think this is a symptom of a bigger issue, which is we aren't properly handling very fast user input or even debouncing it. In regular circumstances, it should be fine. I think we can release this after my comments above are addressed.

GioBonvi commented 7 years ago

What I am seeing is that if I switch back and forth between desktops, some of which are named like this, sometimes the default name appears instead (i.e. Desktop N).

I could not replicate this behavior even swithching at the highest speed.

I think this is a symptom of a bigger issue, which is we aren't properly handling very fast user input or even debouncing it.

I believe the fade-out animation of the tootltips could be the culprit here: when I coded that feature I ran some tests with huge fade-out times (like 2 seconds or more) and if I rememeber correctly the whole program is unresponsive during the animation...
We should probably test this: maybe removing the fade-out would solve these issues.

sdias commented 7 years ago

I could not replicate this behavior even swithching at the highest speed.

I tried a mix of switch to desktop N and switch to prev/next desktop, using the default configs.

I believe the fade-out animation of the tootltips could be the culprit here: when I coded that feature I ran some tests with huge fade-out times (like 2 seconds or more) and if I rememeber correctly the whole program is unresponsive during the animation...
We should probably test this: maybe removing the fade-out would solve these issues.

Potentially, yes, but I wouldn't be surprised if the time that all of the internal logic takes to run (excluding fading out tooltips) for every switch was not too much in some situations. We should really debounce any user input. I don't think you should address that in this branch though, or even do it now.

GioBonvi commented 7 years ago

Merging this then