Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display info icon for Initialized/Inactive extension state #656

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

TanmayPatil105
Copy link
Contributor

@TanmayPatil105 TanmayPatil105 commented May 25, 2024

Updates

image
image

Related issues

#646

@oscfdezdz
Copy link
Collaborator

Thank you for looking into this! I think we don't need to add some of the states as tooltips since we show icons with tooltips with more detailed explanation. These would exclude ERROR and OUT_OF_DATE states, a screenshot of the latter:

image

Also, it seems that the states of the extensions in the different distributions don't work the same. In Fedora, for the same extensions you have shown, I see:

image
image

I'm not quite sure why this happens, but it makes things a little more difficult for us to try to improve how the status is displayed to the user.

@oscfdezdz
Copy link
Collaborator

Reviewing all states, the transient states ACTIVATING and DEACTIVATING are also dispensable, the switch transition is self-explanatory and short enough that it isn't necessary. And with ACTIVE state, the active switch with the accent color highlighting also makes it redundant.

That leaves us with INACTIVE and INITIALIZED, while the former should be clear because of the deactivated switch, it seems that it isn't the case on all systems. Still, with these two states being the only ones causing confusion, I'd prefer that we add the tooltip to an icon as in the case of ERROR and OUT_OF_DATE, if that's ok with you.

You can see the details of the implementation of the icons in that same source file and in exm-extension-row.blp. Using an icon like info-outline-symbolic should do it. Although it still remains to find out, as mentioned before, why the states are different depending on the system. I'll try to look into it in the next few days.

@TanmayPatil105
Copy link
Contributor Author

Also, it seems that the states of the extensions in the different distributions don't work the same. In Fedora, for the same extensions you have shown, I see:

Yeah, that's weird. I'll look into that too!!

Thank you for looking into this! I think we don't need to add some of the states as tooltips since we show icons with tooltips with more detailed explanation. These would exclude ERROR and OUT_OF_DATE states, a screenshot of the latter:

Reviewing all states, the transient states ACTIVATING and DEACTIVATING are also dispensable, the switch transition is self-explanatory and short enough that it isn't necessary. And with ACTIVE state, the active switch with the accent color highlighting also makes it redundant.

That makes sense.

You can see the details of the implementation of the icons in that same source file and in exm-extension-row.blp. Using an icon like info-outline-symbolic should do it. Although it still remains to find out, as mentioned before, why the states are different depending on the system. I'll try to look into it in the next few days.

Sounds good. I'll update the patch.
Thanks!

@TanmayPatil105
Copy link
Contributor Author

Not related to this PR but while viewing an Extension details the reviews are getting leaked onto the terminal.

tanmay@tanmay-lenovo:~/Desktop/projects/gnome/extension-manager$ ./_build/src/extension-manager
<p>Works on Nobora 39</p>
Root Node discovered: html
Ignored element html
Ignored element body
<p>Not working on Ubuntu 22.04.4</p>
Root Node discovered: html
Ignored element html
Ignored element body
<p>Works!!!!!<br>Thank You!!!!!!!!!!!!!!!!!!!!!!</p>
Root Node discovered: html
Ignored element html
Ignored element body
<p>Woks great!</p>
Root Node discovered: html
Ignored element html
Ignored element body
<p>Great Extension, now working on Gnome 46, thank you</p>
Root Node discovered: html
Ignored element html
Ignored element body

That must be due to a stray printf?

@oscfdezdz
Copy link
Collaborator

Yes, that's #361. I looked at it a while ago and it's from text-engine.

@TanmayPatil105
Copy link
Contributor Author

image

@TanmayPatil105
Copy link
Contributor Author

@oscfdezdz Please take a look!

@TanmayPatil105 TanmayPatil105 changed the title Set the extension state as a tooltip for extension toggle button Display info icon for Initialized/Inactive extension state May 28, 2024
@oscfdezdz
Copy link
Collaborator

oscfdezdz commented May 28, 2024

It's looking great, thank you very much! 😄

While we find the cause of the difference of states between systems, you can check if the INACTIVE state and GtkSwitch properties active is true and state is false (systems like yours) and the INITIALIZED state and GtkSwitch properties active is true and state is false (systems like mine). That will display only in extensions like "Control Blur Effect On Lockscreen" or "Customize Clock on Lock Screen" on both systems without difference.

@TanmayPatil105
Copy link
Contributor Author

INACTIVE:
image

@TanmayPatil105
Copy link
Contributor Author

I figured something out:
After booting up, the state of the extensions is INITIALIZED.
If we lock and then unlock the screen, it becomes INACTIVE.

Could you check if this is reproducible?

@oscfdezdz
Copy link
Collaborator

That's it, I can reproduce it. There are no differences then.

Sorry if I didn't explain myself well in my last comment 😅 I meant that we should check the status and active properties of the switch in the code to show the icon, in those specific cases. Now that I think about it again, I don't think it would be necessary, with the enabled variable it should be enough.

In the screenshot you sent there were manually disabled extensions, such as DING or Ubuntu Dock that have the icon visible and should not.

Now that you have figured out why the states were different, something like:

gtk_widget_set_visible (GTK_WIDGET (self->info_icon),
                            (state == EXM_EXTENSION_STATE_INITIALIZED
                            || state == EXM_EXTENSION_STATE_INACTIVE)
                            && enabled);

should work, I haven't test it.

I'd also now use one tooltip instead (could be set in blp file directly) with an understandable message for all users. I was thinking of:

This extension could be activated in session modes such as the login screen or the lock screen

@TanmayPatil105
Copy link
Contributor Author

Done, Thanks!

Copy link
Collaborator

@oscfdezdz oscfdezdz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for raising the issue and fixing it! From my side all is good, I'll let @mjakeman have the last word.

@oscfdezdz oscfdezdz requested a review from mjakeman May 30, 2024 18:54
@mjakeman
Copy link
Owner

mjakeman commented Jun 5, 2024

Hi @TanmayPatil105, thanks for the contribution!

I think as a general philosophy, the app shouldn't have a state where a tooltip/icon is needed to explain why a "core feature" does not work intuitively. We should either hide the toggle, or bind it to a different property. In this case, the toggle really should be bound to enabled rather than active going off the issue you raised for shell.

However I also think that perfect is the enemy of good, and this will definitely be useful in the short term.

I'm happy to merge for now, with a long term view to redo how we handle enabled/active :)

@mjakeman mjakeman merged commit 885b8a6 into mjakeman:master Jun 5, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants