brute4s99 added inline comments.
INLINE COMMENTS
> broulik wrote in notifybysnore.cpp:200
> Does the notification still end up with an `id` eventually?
yes. I've used the id in the new patch
REPOSITORY
R289 KNotifications
REVISION DETAIL
https://phabricator.kde.org/D26801
To: brute4s99,
brute4s99 abandoned this revision.
REPOSITORY
R289 KNotifications
REVISION DETAIL
https://phabricator.kde.org/D26801
To: brute4s99, vonreth, broulik, #kde_connect
Cc: anthonyfieroni, kde-frameworks-devel, nalvarez, KunalRaghav, ankitbaluni,
ankit, aliencode, Orage, ritwizsinha, LeGast00n,
brute4s99 marked 2 inline comments as done.
brute4s99 added a comment.
I will be closing this diff as I recently formed a simpler patch. Inviting
your reviews on that one!
https://phabricator.kde.org/D26888
REPOSITORY
R289 KNotifications
REVISION DETAIL
broulik added inline comments.
INLINE COMMENTS
> notifybysnore.cpp:89
> +KNotification *notification = m_notifications.value(id);
> +if (notification == nullptr) {
> qCDebug(LOG_KNOTIFICATIONS) << "Notification not found!";
`if (!notification) {`
>
brute4s99 updated this revision to Diff 74190.
brute4s99 marked 2 inline comments as done.
brute4s99 added a comment.
updated
REPOSITORY
R289 KNotifications
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D26801?vs=74005=74190
BRANCH
arcpatch-D26801
REVISION DETAIL
vonreth added inline comments.
INLINE COMMENTS
> notifybysnore.cpp:90
> +KNotification *notification = m_notifications.value(id);
> +if (notification == 0) {
> qCDebug(LOG_KNOTIFICATIONS) << "Notification not found!";
if (!notification) or at least compare with
brute4s99 marked an inline comment as done.
REPOSITORY
R289 KNotifications
REVISION DETAIL
https://phabricator.kde.org/D26801
To: brute4s99, vonreth, broulik, #kde_connect
Cc: anthonyfieroni, kde-frameworks-devel, nalvarez, KunalRaghav, ankitbaluni,
ankit, aliencode, Orage, ritwizsinha,
brute4s99 updated this revision to Diff 74005.
brute4s99 marked 3 inline comments as done.
brute4s99 added a comment.
update the diff.
REPOSITORY
R289 KNotifications
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D26801?vs=73983=74005
BRANCH
master
REVISION DETAIL
vonreth added inline comments.
INLINE COMMENTS
> anthonyfieroni wrote in notifybysnore.cpp:143
> So the problem isn't here. notification id should be always valid and not
> calculated here.
I'm not sure what would be correct here but just ignoring those notifications
sounds like a bad idea.
anthonyfieroni added inline comments.
INLINE COMMENTS
> brute4s99 wrote in notifybysnore.cpp:143
> KNotif objects by default have `d->id = -1`
>
> `notify()` is invoked by the base with -1 ID and later updated with the
> correct ID and the actions.
>
> Weirdly though, `pairingRequest`
brute4s99 added inline comments.
INLINE COMMENTS
> brute4s99 wrote in notifybysnore.cpp:143
> KNotif objects by default have `d->id = -1`
>
> `notify()` is invoked by the base with -1 ID and later updated with the
> correct ID and the actions.
>
> Weirdly though, `pairingRequest`
brute4s99 added inline comments.
INLINE COMMENTS
> notifybysnore.cpp:143
> {
> -Q_UNUSED(config);
> -// HACK work around that notification->id() is only populated after
> returning from here
> -// note that config will be invalid at that point, so we can't pass that
> along
> -
brute4s99 created this revision.
brute4s99 added reviewers: vonreth, broulik, KDE Connect.
brute4s99 added a project: KDE Connect.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
brute4s99 requested review of this revision.
REVISION SUMMARY
This patch fixes
13 matches
Mail list logo
