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.

Sign up for GitHub

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

Jump to bottom

Rework domains widget #235

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Rework domains widget #235

wants to merge 3 commits into from

Conversation

fepitre
Copy link
Member

@fepitre fepitre commented Nov 30, 2024 •
edited
Loading

@fepitre fepitre changed the title Devel301124 Rework domains widget Nov 30, 2024
@fepitre fepitre force-pushed the devel301124 branch 4 times, most recently from 7ac6e9b to 15c3e12 Compare November 30, 2024 17:24
@codecov Codecov
Copy link

codecov bot commented Nov 30, 2024

Codecov Report

Attention: Patch coverage is 97.06129% with 70 lines in your changes missing coverage. Please review.

Project coverage is 93.43%. Comparing base (ab56644) to head (559df60).

Files with missing lines Patch % Lines
qubes_config/global_config/thisdevice_handler.py 75.00% 11 Missing ⚠️
qubes_config/global_config/policy_rules.py 87.09% 8 Missing ⚠️
qubes_config/new_qube/new_qube_app.py 88.88% 6 Missing ⚠️
qubes_config/policy_editor/policy_editor.py 94.44% 5 Missing ⚠️
qubes_config/tests/conftest.py 95.57% 5 Missing ⚠️
...bes_config/tests/test_policy_exceptions_handler.py 91.37% 5 Missing ⚠️
qubes_config/tests/test_policy_handler.py 93.58% 5 Missing ⚠️
qubes_config/global_config/policy_handler.py 93.10% 4 Missing ⚠️
...g/tests/test_new_qube/test_application_selector.py 95.18% 4 Missing ⚠️
qubes_config/global_config/global_config.py 93.02% 3 Missing ⚠️
... and 7 more
Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #235   +/-   ##
=======================================
  Coverage   93.43%   93.43%           
=======================================
  Files          57       57           
  Lines       10997    10997           
=======================================
  Hits        10275    10275           
  Misses        722      722

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alimirjamali
alimirjamali reviewed Nov 30, 2024
View reviewed changes
Copy link

@alimirjamali alimirjamali left a comment

Choose a reason for hiding this comment

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

Technically both of Pylint checks and Black formatting style are enforced here? If that is the case, it might be better to update Python specific Coding Style Guidelines as well. There is a brief line there with a link to PEP8.

alimirjamali
alimirjamali reviewed Nov 30, 2024
View reviewed changes
qui/devices/device_widget.py

self.widget_icon = Gtk.StatusIcon()
self.widget_icon.set_from_icon_name('qubes-devices')
self.widget_icon.connect('button-press-event', self.show_menu)
self.widget_icon.set_from_icon_name("qubes-devices")
Copy link

Choose a reason for hiding this comment

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

While you are at it, it might be a good opportunity to take a loot at issue #9208 as well. Workaround should be as easy and saving the qubes-devices svg file as a 256x256 svg. Or maybe providing the icon as PNG files of different sizes as well. I do not have i3wm installed and could not verify the outcome myself.

Copy link
Member Author

@fepitre fepitre Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I cannot reproduce this issue because I don't really see what's wrong. For me, the domain or device widget tray icon is exactly the same than other icons even if I increase the bar size.

@fepitre
Copy link
Member Author

fepitre commented Dec 2, 2024

Technically both of Pylint checks and Black formatting style are enforced here? If that is the case, it might be better to update Python specific Coding Style Guidelines as well. There is a brief line there with a link to PEP8.

Yes, but it's not yet enforced in guidelines. It's up to @marmarek but for now we have several other places to blacken first.

marmarta
marmarta suggested changes Dec 11, 2024
View reviewed changes
Copy link
Member

@marmarta marmarta left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There's something very wrong with the margins here, I'm afraid:

  • your changes:
    fepitre-qui-domains
  • previous state
    fepitre-qui-domains-2

I'm not against some more margins, but this is too much ;)

qui/tray/domains.py

gi.require_version("Gtk", "3.0") # isort:skip
from gi.repository import Gio, Gtk, GObject, GLib, GdkPixbuf # isort:skip
from gi.repository import Gio, Gtk, GLib, GdkPixbuf # isort:skip
Copy link
Member

Choose a reason for hiding this comment

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

please don't move import gi away from this, it makes it more confusing...

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean? This dependency is not required anymore, how should I remove it?

qui/tray/domains.py
"""
raise NotImplementedError("Subclasses must implement this method.")
Copy link
Member

Choose a reason for hiding this comment

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

why not use the abc.abstractmethod annotation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I need to retry, I think there were some complain about mypy at first.

@fepitre
Copy link
Member Author

fepitre commented Dec 11, 2024

There's something very wrong with the margins here, I'm afraid:

* your changes:
  ![fepitre-qui-domains](https://private-user-images.githubusercontent.com/7242298/394781409-1d9e70ce-46df-4429-8f4a-22c0f88b8e94.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzM5Mjk4ODMsIm5iZiI6MTczMzkyOTU4MywicGF0aCI6Ii83MjQyMjk4LzM5NDc4MTQwOS0xZDllNzBjZS00NmRmLTQ0MjktOGY0YS0yMmMwZjg4YjhlOTQucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MTIxMSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDEyMTFUMTUwNjIzWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ODU1ZmJlOThhOTllYmE5MjQ5ZGNjZDFlMTg5ZWVhN2E5YTg2MmYwNDE3MjNmMmJjZmFiYjNkMTcwYTA1ZWNkOSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.8yWiCmOvdRWkipa5ED5KHmxN2EqsNfXyb7GTJacIS4g)

* previous state
  ![fepitre-qui-domains-2](https://private-user-images.githubusercontent.com/7242298/394781416-86eb95a8-79e7-4cfe-a4a0-ef982f86a876.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzM5Mjk4ODMsIm5iZiI6MTczMzkyOTU4MywicGF0aCI6Ii83MjQyMjk4LzM5NDc4MTQxNi04NmViOTVhOC03OWU3LTRjZmUtYTRhMC1lZjk4MmY4NmE4NzYucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MTIxMSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDEyMTFUMTUwNjIzWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NzEyMjBjODBkNWJhYzZmYTc0ZGQ0Yjk4YTQ2NWExYzgyYzdkOGY5MjA1NDcyZjM2MWQ5NmFlMDVlNDZjZGI1NiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.6nLSkJ2IAIY_zFWB2QMWeF5avvKaerlNqqYaY8sOD9A)

I'm not against some more margins, but this is too much ;)

Yes I'm aware of this but I was waiting on current refactor before fighting with margins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alimirjamali alimirjamali alimirjamali left review comments

@marmarta marmarta marmarta requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Icons are not being displayed under Qubes Domains and Qubes Devices widgets on i3 WM
3 participants
@fepitre @alimirjamali @marmarta