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

Fix notifications #521

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

sh-zam
Copy link
Contributor

@sh-zam sh-zam commented Jan 24, 2020

This should most make the notifications work like it worked in past. (However, it doesn't use the pynotify).

There is one issue, the path to the icon is hardcoded. I am not sure, if we can dynamically access the installed location of hamster or the "future" installation structure of hamster.

CC: issue#493

@sh-zam sh-zam requested a review from ederag January 24, 2020 15:23
@ederag
Copy link
Collaborator

ederag commented Jan 24, 2020

Thanks, this looks very interesting.
But for reasons explained in #493, this should be external (not in hamster itself):

All these features can be implemented separately,
talking to the main hamster through dbus, for instance.

Please do ask there for any clarifications.

The next weeks free time will be devoted to finalizing v3.0,
only then will I be able to help setting up an external service.

Meanwhile, please do keep this PR open.
If you merge master into it regularly, people will be able to use it (instructions),
until the external companion is up and running.

About the icon location, the base path can be accessed in defs:

>>> from hamster import defs
>>> defs.DATA_DIR
'/usr/share'

But that's about it. The new icon installation path is set in

bld.install_files('${DATADIR}/hamster/art', start_dir.ant_glob('art/*.png'))
bld.install_files('${DATADIR}/hamster', 'report_template.html')
bld.install_files('${DATADIR}/icons/hicolor/16x16/apps', 'art/16x16/hamster.png')
bld.install_files('${DATADIR}/icons/hicolor/22x22/apps', 'art/22x22/hamster.png')
bld.install_files('${DATADIR}/icons/hicolor/32x32/apps', 'art/32x32/hamster.png')
bld.install_files('${DATADIR}/icons/hicolor/48x48/apps', 'art/scalable/hamster.png')
bld.install_files('${DATADIR}/icons/hicolor/scalable/apps','art/scalable/hamster.svg')

The directory and names changed in #485,
so beware that it seems to work on your system because of remnants from a previous installation.

There should be a better way though, for instance in the ui, icons can be found by name:

<property name="icon_name">hamster</property>

@ederag ederag removed their request for review January 25, 2020 14:24
@sh-zam sh-zam force-pushed the fix-notifications branch 3 times, most recently from d558d34 to 88eb65f Compare February 18, 2020 13:34
@projecthamster projecthamster deleted a comment from CLAassistant Mar 6, 2020
@projecthamster projecthamster deleted a comment from ederag Apr 28, 2020
@ederag
Copy link
Collaborator

ederag commented Apr 28, 2020

There will never be any CLA on this code. Please disregard the wrong checks (#589).

The code for notifications was removed somewhere in the history.
With this commit, notifications are expected to work as usual!

Signed-off-by: Sharaf Zaman <[email protected]>
@sh-zam
Copy link
Contributor Author

sh-zam commented May 17, 2020

I have updated this to the current master and I have updated the way to get icon path, however I am not sure if this is how you're supposed to do it.

Copy link
Contributor

@mwilck mwilck left a comment

Choose a reason for hiding this comment

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

The code looks good in general.

I'd like to understand how it relates to the previous code removed in 08eaf77 (desktop.py). Have you used that code as a template / reference somehow? You seem to have implemented some things differently (looking at the actual Notify() code for example - missing transient and urgency properties), could you elaborate a bit on that, either in the commit message or in comments in the code?

@@ -46,7 +46,7 @@
from hamster.lib import default_logger, stuff
from hamster.lib import datetime as dt
from hamster.lib.fact import Fact

from hamster.lib.notifsmanager import notifs_mgr
Copy link
Contributor

Choose a reason for hiding this comment

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

notifs_mgr seems to be referenced nowhere in this file. What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Must be remnants of my debugging session. I'll remove it, thanks!

@sh-zam
Copy link
Contributor Author

sh-zam commented May 18, 2020

Thank you for reviewing!

I'd like to understand how it relates to the previous code removed in 08eaf77 (desktop.py). Have you used that code as a template / reference somehow?

No, I did not I'm afraid. I wrote it from scratch by looking at the docs :(

You seem to have implemented some things differently (looking at the actual Notify() code for example - missing transient and urgency properties), could you elaborate a bit on that, either in the commit message or in comments in the code?

I didn't know that such a thing (transient) existed. I will update my patch to include it and add a few comments along the way.

PS: Just yesterday, I added code to close notifications automatically. It was an ugly hack. Thanks for mentioning transient :-)

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.

3 participants