jaszhix / icingtaskmanager

Window list with app grouping and thumbnails for Cinnamon
https://cinnamon-spices.linuxmint.com/applets/view/269
GNU General Public License v2.0
60 stars 12 forks source link

Active app button stuck in "hover" state #199

Closed Singond closed 6 years ago

Singond commented 6 years ago

I'm using Ubuntu 16.04 with Cinnamon 3.6.7 and ITM 6.3.1, but I remember seeing the bug in earlier versions as well.

I have an issue where the hover state of an active window icon is not being updated as expected and remains on the button even after the mouse leaves it. This happens anytime I mouse over the button of an app which is currently in focus.

To reproduce this:

  1. Use a Cinnamon theme in which the hover effect is visible and easily distinguishable from the other effects like active state (the Carta theme works well: Carta).
  2. In ITM preferences, under Theming, set App button hover pseudo class to hover.
  3. Open any application. Let' call this window App 1.
  4. Open another appplication (different from the first one). Let's call this App 2. We now have two window icons in the task manager, both having the state active.
  5. Focus App 1 (either by clicking the window or through Alt-Tab).
  6. Hover your mouse pointer over App 1 button in ITM. The hover state indication appears as expected.
  7. Move your mouse pointer off the button.
  8. The issue: The button of App 1 still displays the hover state.
  9. Focus App 2 (in any of the aforementioned ways). The button of App 1 is still in hover state.
  10. Hover your mouse pointer over App 1 button again.
  11. Move your mouse pointer off the button. Only now does the hover indication disappear.

These are my log files: glass.log 14.json .xsession-errors

ArionWT commented 6 years ago

Have same bug with:

Button in hover: outlined
Button in focus: focus
Button in active: hover

When turned "Show thumbnail by a click" - issue have gone.

With this options:

Button in hover: hover
Button in focus: focus
Button in active: hover

When hovering mouse on button, blinks 'active' state, and after 'outlined' state, when mouse will moved out button. Cinnamon v3.6.7 Sorry for my broken English.

jaszhix commented 6 years ago

The carta theme works fine for me if you set:

I can't support every combination of pseudo class options for every theme, they actually exist as a workaround for theme incompatibility. The settings above is the "Minty-Y and Variants" preset. Perhaps it, or the Mint X preset should be default since those cover almost all theme issues.

Please confirm whether or not the Mint-Y preset fixes this issue. I'll keep this open until I update the settings to indicate those options are experimental/workaround-only at best.

ArionWT commented 6 years ago

Yes, with Mint-Y everything works perfectly.

Singond commented 6 years ago

Hello jaszhix, thanks for considering the issue.

I have just tested the Mint Y preset, but I am not satisfied. The preset "works" correctly in the sense of doing what it describes: It assigns the hover class to both the focused window icon and the hovered icon. However, I expected to be able to use different classes for the hovered and focused window (in order to keep the two states visually distinguishable). Is there any support for this?

I don't think the problem is with the pseudo classes. To me it seems that when the focus is changed to another window, some event handler in ITM fails to remove the appropriate class from the old focused window and add it to the newly focused one. It only does so after hovering the thumbnails.

I think setting the Focus pseudo class to hover does not solve the issue, but merely hides it, because it changes what is considered the desired result.

jaszhix commented 6 years ago

ITM has to have one extra state the default Cinnamon window list doesn't - apps that are running. Since the default window list only shows running apps, there's no need for distinguishing between closed/pinned and running apps. Most themes target the styling requirements of the default Cinnamon CSS, which only concerns itself with that. This was an issue with the original applet ITM is forked from "Window List With App Grouping". CobiWindowList does this with open window count numbers, but its always been an option in ITM to disable that - a default I chose because it looks cleaner to me.

To properly fix this, we would need to PR a new class into the default Cinnamon CSS. I would rather move in that direction, maybe even give the default window list basic app grouping functionality as a use case for it - instead of patching the fragile pseudo class manipulator code that has to account for 100+ themes.

Martin1887 commented 6 years ago

I attach .xsession_errors to provide information about this issue.

xsession-errors.txt

jaszhix commented 6 years ago

@Martin1887 Thanks. I see a lot of theme parsing errors. I know there were a lot of changes that needed to be made for GTK 3.22. I recall from https://github.com/linuxmint/cinnamon-spices-applets/issues/1781 you have a custom modified theme that is not available anywhere but on your computer? I don't have access to it, and wouldn't be possible to support that scenario for everyone. When you do custom modifications without peer review, any theme related issues will be hard to reproduce, so I'm not sure its valid.

Singond commented 6 years ago

Hello Jason, I have managed to solve the issue to my liking. I found the culprit to be the AppGroup._onLeave function, which returned early if the app (whose icon the mouse pointer had just left) was in focus. I removed the check for focus state (along with the early return) and the applet now behaves according to my expectations. I tested setting the App button hover pseudo class to both hover and outlined and saw positive behaviour in both cases.

I don't think theme compatibility is an issue here. The aforementioned function simply removes whatever class is assigned to hover state, the exception being when the same class is used for another state like focus. My patch merely ensures this behaviour is triggered for all app icons, as opposed to non-focused apps only. I am opening a pull request for your reference.

jaszhix commented 6 years ago

@Singond Hi, while this may fix your particular issue with Carta, it regresses the fix for #189 and #190. I would be more leaning towards making this an option of some sort, but I don't think its fixing the root cause of this (with the focus code). Again, I am probably going to solve the bugs caused by the theme workarounds by adding correct theme support into Cinnamon. I also am working on the 3.8 branch currently.

jaszhix commented 6 years ago

@Singond I think I have a fix for this that doesn't regress the older issues. Can you test this please? https://github.com/jaszhix/applets/commit/b7fc44a43db431ad0c63c09bb6fdfd9745d0b98a

Singond commented 6 years ago

I have tested the fix at focus-hover-fix branch, but the problem persists. In the resetHoverStatus function, if one of the app windows is in focus, the function terminates in the loop and the line

this.actor.remove_style_pseudo_class(hoverPseudoClass);

never gets called, leaving the icon marked with hoverPseudoClass.

jaszhix commented 6 years ago

@Singond Is your settings JSON any different? How would I go about reproducing this?

Edit: The change I made was adding a call to resetHoverStatus in the focus change handler. I've been reproducing your issue up until my recent commit.

Singond commented 6 years ago

@jaszhix I see, you made changes to the _onFocusChange function as well.

This is my .json file located at ~/.cinnamon/configs/IcingTaskManager@json/. Is this the file you mean? settings.json.txt

I'm testing with the Carta theme and the procedure is the same as in my first post: Focus an app; no app icon is set to hover; now hover over all the app icons in ITM (without clicking them). The app icon of the focused app retains the hover state.

jaszhix commented 6 years ago

@Singond It could be an issue on Cinnamon 3.6... I haven't been able to test it as I've been compiling Cinnamon git for months. This commit is likely the last one for the 3.4-3.6 compatibility branch.

Edit - I do test in VM for big changes, but for the most part have been working on 3.8.

jaszhix commented 6 years ago

@Singond Pushed another commit. I think the misunderstanding is that Carta's focus and active psuedo classes are styled the same, so when you have "Show active app indicators" enabled, it uses the same style for running apps, so you can't distinguish what is focused from the taskbar with your current settings. This is why the Mint Y preset is there, and why we need to add support in Cinnamon.

On the other hand there was still a bug where the hover class wasn't being removed after a click event on hover leave, that's fixed in https://github.com/jaszhix/applets/commit/2d7de4dac5e2c07026acc17ccced874a5b925a70. If you don't care about running app indicators you can disable that setting and the focus style will be noticeable, or try to find a different pseudo class for active.

jaszhix commented 6 years ago

Went ahead and merged it, I don't think this issue can improve any further. Contact the theme author if you'd like it to be more compatible with ITM.

gabrielfern commented 6 years ago

hey @jaszhix , this new commit brings the issue https://github.com/jaszhix/icingtaskmanager/issues/189 back

jaszhix commented 6 years ago

@gabrielfern Looking at it, it doesn't seem like the newest changes are causing it. I isolated the cause to the call of PopupMenu.PopupMenu.prototype.open in AppThumbnailHoverMenu.open, so the change is likely caused by a change in Cinnamon. Inside PopupMenu.prototype.open if I comment out this line this.emit('open-state-changed', true);, the focus pseudo class persists. The only signal connected to that in ITM is the toggler for the context menu, which makes no difference when commented out. Will need to look into it more.

gabrielfern commented 6 years ago

@jaszhix I just get the newer version today, and noticed the problem just after updating it, so I came to github and I got the repository (linuxmint/cinnamon-spices-applets) in the last commit before this commit and copy/paste the contents of appGroup.js in my ITM and the problem dissapeared. So I think the problem is back because these modifications (the last commit). I dont know more about it, sorry :(

jaszhix commented 6 years ago

@gabrielfern Its happening when I revert to the previous commit for me though, using the settings you use in #189. Can't go back too much further without deleting the 3.8 directory, since its addition is new.

gabrielfern commented 6 years ago

@jaszhix I use with the Mint x/LinuxMint preset, and it works fine