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

Cleaner notifications #49

Merged
merged 3 commits into from
Oct 20, 2024
Merged

Conversation

b100dian
Copy link
Contributor

As per @nephros' pointers:

  • Removed the notificator code
  • Added simpler notification creation

Also, when "tryToBeSilent" is true, the notification is still published (count updated) but does not ping you in hydrogen.

Last minute fix: this should honor Sticky notifs settings too.

@b100dian
Copy link
Contributor Author

I've also updated chum:testing with this branch if you want to test, this will be the next release

var tryToBeSilent = Qt.application.state == Qt.ApplicationActive;
var message = notificationComponent.createObject(null, {
'previewSummary': tryToBeSilent ? null: qsTr("New hydrogen message."),
'isTransient': !appConfig.stickyNotifications,
Copy link
Contributor

@nephros nephros Oct 18, 2024

Choose a reason for hiding this comment

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

AFAIK, isTransient mainly causes the notification to be a popup, and not show up in the Event view.

The intent of "Sticky" notifications (which only existed as a Setting and were never implemented) was to have a notification on the Event view, but only one, as opposed to a new one each time a notification is triggered.

The handling of lastNotificationId as implemented here should do that, but it may be "defeated" by isTransient - i.e. it may be "sticky" but never reach the Event View because transient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a very good point - I just re-read the descripton from UI for "Sticky notifications".

In my opinion, since we don't (yet) deliver a notification per room, having them sticky by default is useful at the moment.
With that in mind, I thought that making them transient would be the other option.,

Now: what do you think - should I re-phrase the UI to my changes (e.g "Transient notifications", or actually make lastNotificationId play along only when sticky (and make that the default)? Third option: just remove UI setting and make all sticky for now. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with re-purposing transient vs sticky in settings

@nephros
Copy link
Contributor

nephros commented Oct 18, 2024

Cool, good riddance to my needlessly overcomplicated version.

@b100dian b100dian merged commit a218183 into hydrogen-sailfishos:master Oct 20, 2024
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.

2 participants