Use PyGObject for NotifyDBus plugin

qarkai commented 11 months ago

Description:

Related issue (if applicable): #984

Disabled some tests for now.

Checklist

[x] The code change is tested and works locally.
[x] There is no commented out code in this PR.
[x] 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 commented 11 months ago

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%.

:exclamation: 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 :warning:
apprise/plugins/NotifyDBus.py	62.50%	6 Missing :warning:

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

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

caronc commented 9 months ago

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

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 commented 9 months ago

@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 commented 9 months ago

@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 commented 9 months ago

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 :slightly_smiling_face: . 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 commented 9 months ago

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 commented 4 months ago

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

amotl commented 4 months ago

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 commented 4 months ago

@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

caronc / apprise