-
Notifications
You must be signed in to change notification settings - Fork 668
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
Show Server Notifications on the Client #4567
Changes from 1 commit
0eb1041
a831b74
eb00b34
688c550
32e16b3
4a4dac2
b97c832
7d13a1d
8a0ce46
903e79a
2d1ab27
2c2a18a
adf9570
73cd5a9
97f1694
9d219a1
9a2f145
a4dcc27
45c32ec
f7f4120
f71fdab
05de710
0a590b7
328d254
7f22a07
b966345
f587f35
d407aac
d03fcc9
ad60e8a
f70c628
ea2f19b
161d219
1bb3a4a
0c944a0
1fe5d6b
69e8e15
4d59f5e
cacb751
9f438cb
2e30a0e
cd3f612
8166c52
885f8b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
The policy that was said is that if a notification has no action, the client can and should display a close-button. This patch does it. In additon to that, the client needs a blacklist of closed notifcations otherwise they would re-appear next time the server notifications are fetched again. Also, changed the cleanup of not-longer-used widgets to be more robust.
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,6 +94,9 @@ ActivityWidget::ActivityWidget(QWidget *parent) : | |
|
||
connect( _ui->_activityList, SIGNAL(activated(QModelIndex)), this, | ||
SLOT(slotOpenFile(QModelIndex))); | ||
|
||
connect( &_removeTimer, SIGNAL(timeout()), this, SLOT(slotCheckToCleanWidgets()) ); | ||
_removeTimer.setInterval(1000); | ||
} | ||
|
||
ActivityWidget::~ActivityWidget() | ||
|
@@ -231,18 +234,26 @@ void ActivityWidget::slotBuildNotificationDisplay(const ActivityList& list) | |
QString listAccountName; | ||
|
||
foreach( auto activity, list ) { | ||
if( _blacklistedActivities.contains(activity)) { | ||
qDebug() << Q_FUNC_INFO << "Activity in blacklist, skip"; | ||
continue; | ||
} | ||
|
||
NotificationWidget *widget = 0; | ||
|
||
if( _widgetForNotifId.contains(activity._id) ) { | ||
widget = _widgetForNotifId[activity._id]; | ||
if( _widgetForNotifId.contains( activity.ident()) ) { | ||
widget = _widgetForNotifId[activity.ident()]; | ||
} else { | ||
widget = new NotificationWidget(this); | ||
connect(widget, SIGNAL(sendNotificationRequest(QString, QString, QString)), | ||
this, SLOT(slotSendNotificationRequest(QString, QString, QString))); | ||
connect(widget, SIGNAL(requestCleanupAndBlacklist(Activity)), | ||
this, SLOT(slotRequestCleanupAndBlacklist(Activity))); | ||
|
||
_notificationsLayout->addWidget(widget); | ||
// _ui->_notifyScroll->setMinimumHeight( widget->height()); | ||
_ui->_notifyScroll->setSizeAdjustPolicy(QAbstractScrollArea::AdjustToContentsOnFirstShow); | ||
_widgetForNotifId[activity._id] = widget; | ||
_widgetForNotifId[activity.ident()] = widget; | ||
} | ||
|
||
widget->setActivity( activity ); | ||
|
@@ -280,19 +291,20 @@ void ActivityWidget::slotBuildNotificationDisplay(const ActivityList& list) | |
} | ||
} | ||
|
||
// check if we have widgets that have no corresponding activity from | ||
// check if there are widgets that have no corresponding activity from | ||
// the server any more. Collect them in a list | ||
QList<int> strayCats; | ||
QList< Activity::Identifier > strayCats; | ||
foreach( auto id, _widgetForNotifId.keys() ) { | ||
bool found = false; | ||
NotificationWidget *widget = _widgetForNotifId[id]; | ||
|
||
bool found = false; | ||
// do not mark widgets of other accounts to delete. | ||
if( widget->accountName() != listAccountName ) { | ||
if( widget->activity()._accName != listAccountName ) { | ||
continue; | ||
} | ||
|
||
foreach( auto activity, list ) { | ||
if( activity._id == id ) { | ||
if( activity.ident() == id ) { | ||
// found an activity | ||
found = true; | ||
break; | ||
|
@@ -307,8 +319,7 @@ void ActivityWidget::slotBuildNotificationDisplay(const ActivityList& list) | |
// .. and now delete all these stray cat widgets. | ||
foreach( auto strayCatId, strayCats ) { | ||
NotificationWidget *widgetToGo = _widgetForNotifId[strayCatId]; | ||
widgetToGo->deleteLater(); | ||
_widgetForNotifId.remove(strayCatId); | ||
scheduleWidgetToRemove(widgetToGo, 0); | ||
} | ||
|
||
_ui->_notifyLabel->setHidden( _widgetForNotifId.isEmpty() ); | ||
|
@@ -410,21 +421,60 @@ void ActivityWidget::slotNotifyServerFinished( const QString& reply, int replyCo | |
// Add 200 millisecs to the predefined value to make sure that the timer in | ||
// widget's method readyToClose() has elapsed. | ||
if( replyCode == OCS_SUCCESS_STATUS_CODE ) { | ||
QTimer::singleShot(NOTIFICATION_WIDGET_CLOSE_AFTER_MILLISECS+200, this, SLOT(slotCleanWidgetList())); | ||
scheduleWidgetToRemove( job->widget() ); | ||
} | ||
} | ||
|
||
// blacklist the activity coming in here. | ||
void ActivityWidget::slotRequestCleanupAndBlacklist(const Activity& blacklistActivity) | ||
{ | ||
if ( ! _blacklistedActivities.contains(blacklistActivity) ) { | ||
_blacklistedActivities.append(blacklistActivity); | ||
} | ||
|
||
NotificationWidget *widget = _widgetForNotifId[ blacklistActivity.ident() ]; | ||
scheduleWidgetToRemove(widget); | ||
} | ||
|
||
void ActivityWidget::slotCleanWidgetList() | ||
void ActivityWidget::scheduleWidgetToRemove(NotificationWidget *widget, int milliseconds) | ||
{ | ||
foreach( int id, _widgetForNotifId.keys() ) { | ||
Q_ASSERT(_widgetForNotifId[id]); | ||
if( _widgetForNotifId[id]->readyToClose() ) { | ||
auto *widget = _widgetForNotifId[id]; | ||
if( !widget ) { | ||
return; | ||
} | ||
// in fife seconds from now, remove the widget. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "five", but it's actually configurable... |
||
QDateTime removeTime = QDateTime::currentDateTime().addMSecs(milliseconds); | ||
|
||
QPair<QDateTime, NotificationWidget*> removeInfo = qMakePair(removeTime, widget); | ||
if( !_widgetsToRemove.contains(removeInfo) ) { | ||
_widgetsToRemove.insert( removeInfo ); | ||
if( !_removeTimer.isActive() ) { | ||
_removeTimer.start(); | ||
} | ||
} | ||
} | ||
|
||
// Called every second to see if widgets need to be removed. | ||
void ActivityWidget::slotCheckToCleanWidgets() | ||
{ | ||
// loop over all widgets in the to-remove queue | ||
foreach( auto toRemove, _widgetsToRemove ) { | ||
QDateTime t = toRemove.first; | ||
NotificationWidget *widget = toRemove.second; | ||
|
||
if( QDateTime::currentDateTime() > t ) { | ||
// found one to remove! | ||
Activity::Identifier id = widget->activity().ident(); | ||
_widgetForNotifId.remove(id); | ||
delete widget; | ||
widget->deleteLater(); | ||
_widgetsToRemove.remove(toRemove); | ||
} | ||
} | ||
|
||
if( _widgetsToRemove.isEmpty() ) { | ||
_removeTimer.stop(); | ||
} | ||
|
||
// check to see if the whole notification pane should be hidden | ||
if( _widgetForNotifId.isEmpty() ) { | ||
_ui->_notifyLabel->setHidden(true); | ||
_ui->_notifyScroll->setHidden(true); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,7 @@ public slots: | |
void slotRefreshNotifications(AccountState *ptr); | ||
void slotRemoveAccount( AccountState *ptr ); | ||
void slotAccountActivityStatus(AccountState *ast, int statusCode); | ||
void slotRequestCleanupAndBlacklist(const Activity& blacklistActivity); | ||
|
||
signals: | ||
void guiLog(const QString&, const QString&); | ||
|
@@ -80,7 +81,8 @@ private slots: | |
void slotNotifyNetworkError( QNetworkReply* ); | ||
void slotNotifyServerFinished( const QString& reply, int replyCode ); | ||
void endNotificationRequest(NotificationWidget *widget , int replyCode); | ||
void slotCleanWidgetList(); | ||
void scheduleWidgetToRemove(NotificationWidget *widget, int milliseconds = 4500); | ||
void slotCheckToCleanWidgets(); | ||
|
||
private: | ||
void showLabels(); | ||
|
@@ -89,9 +91,13 @@ private slots: | |
QPushButton *_copyBtn; | ||
|
||
QSet<QString> _accountsWithoutActivities; | ||
QMap<int, NotificationWidget*> _widgetForNotifId; | ||
QMap<Activity::Identifier, NotificationWidget*> _widgetForNotifId; | ||
QElapsedTimer _guiLogTimer; | ||
QSet<int> _guiLoggedNotifications; | ||
ActivityList _blacklistedActivities; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this needs to be a QSet otherwise your code might get very slow if there are many blacklisted activities (O(n²)) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, the "blacklisted activities" are actually these notifications that come without any action, which means that they do not display a button. These are only a few. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't persist this information - does it mean users have to acknowledge the server notification every time the client is restarted? I assume that for notifications with actions, the server will keep track of them and that's how we avoid presenting them to the user multiple times... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we present the notifications from the server again, IF the notification is of the special form that has no action, ie. no button defined. I think that is the desired behaviour, as the admin will disable the notifification if it is not longer important. |
||
|
||
QSet< QPair<QDateTime, NotificationWidget*> > _widgetsToRemove; | ||
QTimer _removeTimer; | ||
|
||
// number of currently running notification requests. If non zero, | ||
// no query for notifications is started. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,13 +54,22 @@ void NotificationWidget::setActivity(const Activity& activity) | |
foreach( auto button, _ui._buttonBox->buttons() ) { | ||
_ui._buttonBox->removeButton(button); | ||
} | ||
_buttons.clear(); | ||
|
||
// display buttons for the links | ||
foreach( auto link, activity._links ) { | ||
QPushButton *b = _ui._buttonBox->addButton( link._label, QDialogButtonBox::AcceptRole); | ||
b->setDefault(link._isPrimary); | ||
if( activity._links.isEmpty() ) { | ||
// in case there is no action defined, do a close button. | ||
QPushButton *b = _ui._buttonBox->addButton( QDialogButtonBox::Close ); | ||
b->setDefault(true); | ||
connect(b, SIGNAL(clicked()), this, SLOT(slotButtonClicked())); | ||
_buttons.append(b); | ||
} else { | ||
foreach( auto link, activity._links ) { | ||
QPushButton *b = _ui._buttonBox->addButton(link._label, QDialogButtonBox::AcceptRole); | ||
b->setDefault(link._isPrimary); | ||
connect(b, SIGNAL(clicked()), this, SLOT(slotButtonClicked())); | ||
_buttons.append(b); | ||
} | ||
} | ||
} | ||
|
||
|
@@ -84,12 +93,24 @@ void NotificationWidget::slotButtonClicked() | |
} | ||
|
||
// if the button was found, the link must be called | ||
if( index > -1 && _myActivity._links.count() == 0 ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: You could have a different slot for the close button. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not that easy because the available buttons per notification is not fixed, it is part of the notification description. It has to be dynamic. |
||
// no links, that means it was the close button | ||
// empty link. Just close and remove the widget. | ||
QString doneText = tr("Closing in a few seconds..."); | ||
_ui._timeLabel->setText(doneText); | ||
emit requestCleanupAndBlacklist(_myActivity); | ||
return; | ||
} | ||
|
||
if( index > -1 && index < _myActivity._links.count() ) { | ||
ActivityLink triggeredLink = _myActivity._links.at(index); | ||
_actionLabel = triggeredLink._label; | ||
qDebug() << Q_FUNC_INFO << "Notification Link: "<< triggeredLink._verb << triggeredLink._link; | ||
_progressIndi->startAnimation(); | ||
emit sendNotificationRequest( _accountName, triggeredLink._link, triggeredLink._verb ); | ||
|
||
if( ! triggeredLink._link.isEmpty() ) { | ||
qDebug() << Q_FUNC_INFO << "Notification Link: "<< triggeredLink._verb << triggeredLink._link; | ||
_progressIndi->startAnimation(); | ||
emit sendNotificationRequest( _accountName, triggeredLink._link, triggeredLink._verb ); | ||
} | ||
} | ||
} | ||
} | ||
|
@@ -116,22 +137,11 @@ void NotificationWidget::slotNotificationRequestFinished(int statusCode) | |
|
||
//* The second parameter is a time, such as 'selected at 09:58pm' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. //: |
||
doneText = tr("'%1' selected at %2").arg(_actionLabel).arg(timeStr); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here too |
||
|
||
// start a timer, so that activity widget can remove this widget after a | ||
// certain time. It needs to be done by ActivityWidget because it also | ||
// needs to hide the scrollview if no widget is left any more. | ||
// method readyToClose() checks for the timer value to see if it is expired | ||
_closeTimer.start(); | ||
} | ||
_ui._timeLabel->setText( doneText ); | ||
|
||
_progressIndi->stopAnimation(); | ||
|
||
} | ||
|
||
bool NotificationWidget::readyToClose() | ||
{ | ||
return _closeTimer.isValid() && _closeTimer.elapsed() > NOTIFICATION_WIDGET_CLOSE_AFTER_MILLISECS; | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When are these widgets deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to add that to the TODO list in the first comment.