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

Use PyGObject for NotifyDBus plugin #992

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

qarkai
Copy link

@qarkai qarkai commented Nov 5, 2023

Description:

Related issue (if applicable): #984

Disabled some tests for now.

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • No lint errors (use flake8)
  • 100% test coverage

Testing

Anyone can help test this source code as follows:

# Create a virtual environment to work in as follows:
python3 -m venv apprise

# Change into our new directory
cd apprise

# Activate our virtual environment
source bin/activate

# Install the branch
pip install git+https://github.com/caronc/apprise.git@dbus

# Test out the changes with the following command:
apprise -t "Test Title" -b "Test Message" \
  dbus://

@codecov-commenter
Copy link

codecov-commenter commented Nov 5, 2023

Codecov Report

Attention: 79 lines in your changes are missing coverage. Please review.

Comparison is base (9dcf769) 99.27% compared to head (57ceb32) 98.97%.

❗ Current head 57ceb32 differs from pull request most recent head 31e2a73. Consider uploading reports for the commit 31e2a73 to get more accurate results

Files Patch % Lines
apprise/plugins/NotifyQT.py 40.16% 65 Missing and 8 partials ⚠️
apprise/plugins/NotifyDBus.py 62.50% 6 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #992      +/-   ##
==========================================
- Coverage   99.27%   98.97%   -0.30%     
==========================================
  Files         136      137       +1     
  Lines       17657    17759     +102     
  Branches     3603     3616      +13     
==========================================
+ Hits        17529    17577      +48     
- Misses        119      166      +47     
- Partials        9       16       +7     

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

@caronc
Copy link
Owner

caronc commented Dec 16, 2023

I finally had a chance to look at your code. While i'm sure it works, it doesn't work for me 🙂

The one thing i noticed is your version drops the mapping (qt:// and kde://) and actually just assigns these to the same dbus:// event loop (vs the older version split these apart). Do you know if the KDE event bus is officially gone in DBus v2.0? It's possible it is... (just checking though)?

Edit: I just didn't export DISPLAY=:0 in my dev environment. I take some of my comments back (and cleaned up this response). Your code works fine. Now i just want to handle qt:// and leverage it's message bus if defined (if it still exists).

@caronc
Copy link
Owner

caronc commented Dec 27, 2023

@amotl I would love your amazing skills on the mocker for the new NotifyGLib Notification object. I think it's pretty close as it is very similar/alike to the qlib/dbus libraries you already mocked. I spent a few hours massaging it a bit but have had no luck getting it to go.

@qarkai : I was able to leave the legacy code (renaming it to NotifyDBus) and introducing yours as NotifyGLib (keeping backwards compatibly. Your code launches on glib:// and gio:// to align with your underline library wrapping. I think this is a nice compromise

@qarkai
Copy link
Author

qarkai commented Dec 27, 2023

@caronc Goal of this PR was to get rid of deprecated library. Now it's something different. I don't see much profit in splitting dbus plugins but it's your project.
I don't have pure Qt/KDE environment. Does it have any issues with Gio/PyGObject?

@caronc
Copy link
Owner

caronc commented Dec 27, 2023

I don't have pure Qt/KDE environment. Does it have any issues with Gio/PyGObject?

Yes, your port no longer would support the kde:// and qt:// (alias of kde://)

Goal of this PR was to get rid of deprecated library

@qarkai I personally love your idea and suggestion. Your goal will be achieved; i can assure you this 🙂 . I will absolutely drop the dbus-python requirement, but wil enable the old plugin for those that have it. Simply removing the old plugin removes QT/KDE Support for those using it.

So the idea was to entertain your proposal and additionally support the old one for those who choose to use it (and add dbus-python to their environment. There are some plugins that simply disable themselves if cryptography isn't installed (also not a dependency of Apprise).

I hope this makes sense. The goal is:

Backwards compatiblity:

  • kde://, qt://, dbus:// remain and load through NotifyDBus.py if present.
  • gio:// (new) and glib:// (moved from old library) launch/route through your plugin NotifyGLib.py with less overhead.

Testing is close to working now and @amotl is a genius with the mocker calls as he re-factored my whole test suite a little over a year ago. Hopefully he can offer his two cents and we can merge your great idea.

Chris

@amotl
Copy link
Collaborator

amotl commented Jan 1, 2024

Hi Chris. Thanks for the kind words. Yet, I can't promise anything. So, what would be the procedure you are asking for? Checkout the branch, invoke the tests, and make them succeed? Will it work on macOS, or will I need to use Docker or even a Linux VM for those? Cheers, Andreas.

@amotl
Copy link
Collaborator

amotl commented May 8, 2024

Hi Chris. Unfortunately, I am not running Linux. So, I think I can't be supportive here. Apologies.

@amotl
Copy link
Collaborator

amotl commented May 8, 2024

Testing is close to working now and @amotl is a genius with the mocker calls as he re-factored my whole test suite a little over a year ago. Hopefully he can offer his two cents and we can merge your great idea.

Ah right, you have just been asking about support for proper unit testing with mocking. Currently, it looks like there have been other changes to this plugin. Are the changes here obsolete now, or does the branch need a rebase, in order to enqueue this patch properly again?

@caronc
Copy link
Owner

caronc commented May 11, 2024

@amotl ; i haven't had a chance to revisit this. But yes, Apprise went under a massive file renaming to align better with some inconsistencies between filenames and import (+ mock.patch alignment). This branch will definitely have to be re-based

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.

4 participants