search

AyatanaIndicators / ayatana-indicator-notifications

Ayatana Indicator Notifications Service
GNU General Public License v3.0
4 stars 5 forks source link

Rewrite to indicator-ng #13

Closed tari01 closed 3 years ago

tari01 commented 3 years ago

This should be close enough for an epic merge. Minor tweaks may be needed afterwards.

This pull request does the following:

fixes #9 fixes #12

sunweaver commented 3 years ago

@tari01: it seems that this PR adds several files, but does not remove files properly. I'd expect some files to have moved and changed (e.g. SVG files). However, It seems only files get added, not changed.

Can you re-check that your rewrite contains the same files as the source tree of this PR does?

sunweaver commented 3 years ago

@tari01: please note, the previous approach of the -notification indicator has also been a system indicator, but indicator-object.h old style. Your approach is still a system indicator approach, but going the indicator-ng.h pathway.

Thus, please update your commit messages accordingly. Thanks.

sunweaver commented 3 years ago

notifications

The above screenshot shows that layout tags are not displayed correctly in notifications.

tari01 commented 3 years ago

The above screenshot shows that layout tags are not displayed correctly in notifications.

The menu rendering has been offloaded to ayatana-ido so you need https://github.com/AyatanaIndicators/ayatana-ido/issues/18

sunweaver commented 3 years ago

The above screenshot shows that layout tags are not displayed correctly in notifications.

The menu rendering has been offloaded to ayatana-ido so you need AyatanaIndicators/ayatana-ido#18

Will this menu rendering off-loading work out-of-the-box if other renderers (non-GTK) are used? E.g. via qmenumodel?

tari01 commented 3 years ago

The above screenshot shows that layout tags are not displayed correctly in notifications.

The menu rendering has been offloaded to ayatana-ido so you need AyatanaIndicators/ayatana-ido#18

Will this menu rendering off-loading work out-of-the-box if other renderers (non-GTK) are used? E.g. via qmenumodel?

Whatever displays the menu items, will need to understand x-ayatana-use-markup. This property is currently only used by the new notifications indicator, which is framework-independent. The old one was more like a full-blown GTK application.

tari01 commented 3 years ago

Please see my two previous comments and amend this PR. Thanks!

Ammended. I think it should be allright now.

sunweaver commented 3 years ago

@trism: Hi! Robert Tari rewrote your notifications indicator and turned it from indicator-object.h style to indicator-ng.h style (allowing for more fancy widgets as menu items now). This sort of changes many portions of the code. Robert also ported the build system over to CMake (as we comply to a wish/request of the UBports developers to have all of the system indicators being built with CMake).

@trism, can you go with this rewrite?

As a side note, the settings dialog has been removed from this project as there is now (a brand new) common configuration applet for Ayatana Indicators Settings: https://github.com/AyatanaIndicators/ayatana-settings/

sunweaver commented 3 years ago

@flexiondotorg Please take note of this PR (as already alluded to on Telegram some days ago).

trism commented 3 years ago

It's going to take me a while to go through all the changes, but a couple of quick observations.

sunweaver commented 3 years ago

Hi Jason,

thanks for your feedback. Very much appreciated.

It's going to take me a while to go through all the changes, but a couple of quick observations.

Yes, of course. Thanks for the quick look.

  • Dismissing a notification will close the window every time, which is kind of annoying if you have several hidden notifications to get through.

Yep, agreed. @tari01: Do you see a chance to get this changed to the previous behaviour? (Dismissing notifications keeps the indicator menu open).

  • There is no indication anywhere of how many notifications are remaining, so you have no way of knowing if you have hidden notifications without dismissing them all.

Ah, where was that displayed in the previous version (maybe point us to the position in the code)? We should have that with the rewrite, too, IMHO.

  • It currently marks up urls in the notification summary, but you can't click on the url unless the label inside of the menu item can receive focus, which it cannot.

@tari01: this sounds like it needs to be addressed in IDO, right?

Greets, Mike

tari01 commented 3 years ago
  • Dismissing a notification will close the window every time, which is kind of annoying if you have several hidden notifications to get through.

Yep, agreed. @tari01: Do you see a chance to get this changed to the previous behaviour? (Dismissing notifications keeps the indicator menu open).

The main logic behind this (and some other changes) was to provide the same behaviour for all "clearable" menu lists. Now we have both the notifications and messages indicators work in exactly the same way. Of course, there is nothing stopping us from cooking up a "removable" menu item in IDO. One issue that might arise here is that once you've removed the desired menu items, you would explicitly need to close the menu pane (clicking away, pressing ESC, etc), which is not very indicator-ish.

  • It currently marks up urls in the notification summary, but you can't click on the url unless the label inside of the menu item can receive focus, which it cannot.

@tari01: this sounds like it needs to be addressed in IDO, right?

We can set up an IDO property that tells the menu item whether (and how) to handle URLs (this would probably also break the default click-and-close indicator behaviour).

trism commented 3 years ago

Two more things.

if (self->priv->lVisibleItems == NULL)
{
    setUnread(self, FALSE);
}

after updateClearItem(self) in onRemoveNotification. It wasn't there in the other indicator because I considered closing the menu as having viewed the notifications, so I would clear it then. That's not really going to work if the menu is closed on every dismiss.

Otherwise everything seems to work okay. I am seeing a weird doubled do-not-disturb switch, but I think it might have to do with my ayatana-ido patch being broken somehow. If I remove the switch from the do-not-disturb item, it isn't doubled anymore.

tari01 commented 3 years ago

@trism

Thanks for the review (and sorry for rampaging through your code) :)

@sunweaver

I would prefer we did the merge, getting rid of this mega-PR and address any issues + additional functionality afterwards, with a clean codebase using proper Git history via bug reports + pull requests

sunweaver commented 3 years ago

@sunweaver

I would prefer we did the merge, getting rid of this mega-PR and address any issues + additional functionality afterwards, with a clean codebase using proper Git history via bug reports + pull requests

If @trism is ok with this, I am fine with it, too. @trism: please ACK the above suggestion. Thanks.

trism commented 3 years ago

@tari01 It's fine with me.