D6277: Emit errors when keditbookmarks is missing
This revision was automatically updated to reflect the committed changes. Closed by commit R294:8fb63b082cdd: Emit errors when keditbookmarks is missing (authored by valeriymalov, committed by aacid). REPOSITORY R294 KBookmarks CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6277?vs=15681=15852 REVISION DETAIL https://phabricator.kde.org/D6277 AFFECTED FILES src/kbookmarkmanager.cpp src/kbookmarkmanager.h To: valeriymalov, #frameworks, dfaure Cc: dfaure, aacid, ltoscano
D6277: Emit errors when keditbookmarks is missing
dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Good fix, thanks. INLINE COMMENTS > aacid wrote in kbookmarkmanager.cpp:341 > i'd go for non const as the other two methods, yes they could be const since > they're not modyfing the object, i guess what the original author had in mind > is a meaning of "this changes things eventually" so that's why it isn't > const, or maybe it was just a mistake :D The author is me, and the calling methods are slots (which are typically not const) ;) REPOSITORY R294 KBookmarks REVISION DETAIL https://phabricator.kde.org/D6277 To: valeriymalov, #frameworks, dfaure Cc: dfaure, aacid, ltoscano
D6277: Emit errors when keditbookmarks is missing
valeriymalov updated this revision to Diff 15681. valeriymalov added a comment. Toned down error from critical to warning, removed const cast REPOSITORY R294 KBookmarks CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6277?vs=15608=15681 REVISION DETAIL https://phabricator.kde.org/D6277 AFFECTED FILES src/kbookmarkmanager.cpp src/kbookmarkmanager.h To: valeriymalov, #frameworks Cc: aacid, ltoscano
D6277: Emit errors when keditbookmarks is missing
valeriymalov marked 2 inline comments as done. REPOSITORY R294 KBookmarks REVISION DETAIL https://phabricator.kde.org/D6277 To: valeriymalov, #frameworks Cc: aacid, ltoscano
D6277: Emit errors when keditbookmarks is missing
aacid added inline comments. INLINE COMMENTS > valeriymalov wrote in kbookmarkmanager.cpp:341 > Uh oh, I'd assume startKEditBookmarks should be const (which I forgot) since > it doesn't change the object, yet we need to emit non-const singal error. So > should it be const or not? > > Also, I'm looking at slotEditBookmarks and slotEditBookmarksAtAddress, and > don't understand why those weren't const, since they don't have side-effects > too, although I might be missing something since I'm not too familiar with Qt. i'd go for non const as the other two methods, yes they could be const since they're not modyfing the object, i guess what the original author had in mind is a meaning of "this changes things eventually" so that's why it isn't const, or maybe it was just a mistake :D REPOSITORY R294 KBookmarks REVISION DETAIL https://phabricator.kde.org/D6277 To: valeriymalov, #frameworks Cc: aacid, ltoscano
D6277: Emit errors when keditbookmarks is missing
valeriymalov marked an inline comment as done. valeriymalov added inline comments. INLINE COMMENTS > aacid wrote in kbookmarkmanager.cpp:341 > Why const_cast? Uh oh, I'd assume startKEditBookmarks should be const (which I forgot) since it doesn't change the object, yet we need to emit non-const singal error. So should it be const or not? Also, I'm looking at slotEditBookmarks and slotEditBookmarksAtAddress, and don't understand why those weren't const, since they don't have side-effects too, although I might be missing something since I'm not too familiar with Qt. REPOSITORY R294 KBookmarks REVISION DETAIL https://phabricator.kde.org/D6277 To: valeriymalov, #frameworks Cc: aacid, ltoscano
D6277: Emit errors when keditbookmarks is missing
aacid added inline comments.
INLINE COMMENTS
> kbookmarkmanager.cpp:337
> +if (d->m_dialogAllowed && qobject_cast(qApp) &&
> QThread::currentThread() == qApp->thread()) {
> +QMessageBox::critical(QApplication::activeWindow(),
> QApplication::applicationName(), err);
> +}
critical is a bit heavy in my opinion, i'd just go for warning for this and the
debug line below.
> kbookmarkmanager.cpp:341
> +qCritical() << QStringLiteral("Failed to start keditbookmarks");
> +emit const_cast(this)->error(err);
> +}
Why const_cast?
REPOSITORY
R294 KBookmarks
REVISION DETAIL
https://phabricator.kde.org/D6277
To: valeriymalov, #frameworks
Cc: aacid, ltoscano
D6277: Emit errors when keditbookmarks is missing
valeriymalov marked 2 inline comments as done. valeriymalov added inline comments. INLINE COMMENTS > aacid wrote in kbookmarkmanager.cpp:335 > Have you checked errorString actually returns something? > > Given startDetached is static it seems kind of weird it would update the > "this" errorString Yeah it returns unknown error, my mistake, I suppose I should just switch back to call without object since it's a static method There don't seem to be any informative errors apart from FailedToStart (which is the only one we are interested in?) in QProcess anyway REPOSITORY R294 KBookmarks REVISION DETAIL https://phabricator.kde.org/D6277 To: valeriymalov, #frameworks Cc: aacid, ltoscano
D6277: Emit errors when keditbookmarks is missing
valeriymalov updated this revision to Diff 15608. valeriymalov added a comment. Call startDetached without an object since it's static (my bad, QProcess object does return an error but it's UnknownError and I assume should be seeing FailedToStart; there doesn't seem to be any other interesting errors so probably return value from startDetached should be enough) REPOSITORY R294 KBookmarks CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6277?vs=15589=15608 REVISION DETAIL https://phabricator.kde.org/D6277 AFFECTED FILES src/kbookmarkmanager.cpp src/kbookmarkmanager.h To: valeriymalov, #frameworks Cc: aacid, ltoscano
D6277: Emit errors when keditbookmarks is missing
aacid added inline comments.
INLINE COMMENTS
> kbookmarkmanager.cpp:330
> +{
> +QProcess *keditbookmarks = new QProcess(this);
> +bool success =
> keditbookmarks->startDetached(QStringLiteral("keditbookmarks"), args);
You're leaking keditbookmarks
> kbookmarkmanager.cpp:335
> +QString err = tr("Cannot launch keditbookmarks: %1\n\n"
> + "Most likely you do not have keditbookmarks
> currently installed").arg(keditbookmarks->errorString());
> +
Have you checked errorString actually returns something?
Given startDetached is static it seems kind of weird it would update the "this"
errorString
REPOSITORY
R294 KBookmarks
REVISION DETAIL
https://phabricator.kde.org/D6277
To: valeriymalov, #frameworks
Cc: aacid, ltoscano
D6277: Emit errors when keditbookmarks is missing
valeriymalov edited the summary of this revision. REPOSITORY R294 KBookmarks REVISION DETAIL https://phabricator.kde.org/D6277 To: valeriymalov, #frameworks Cc: ltoscano
D6277: Emit errors when keditbookmarks is missing
ltoscano added a comment. This is not a comment about the content of the patch, but if it closed that bug, please change the reference to the bug with a simple line containing: BUG: xx REPOSITORY R294 KBookmarks REVISION DETAIL https://phabricator.kde.org/D6277 To: valeriymalov, #frameworks Cc: ltoscano
D6277: Emit errors when keditbookmarks is missing
valeriymalov created this revision. valeriymalov added a project: Frameworks. REVISION SUMMARY KBookmarkManager::slotEditBookmarks is supposed to run keditbookmarks, but does not emit any errors when if it's missing This makes "Edit bookmarks" menu fail silently in applications that use KBookmarks (e.g. krdc, konsole) See https://bugs.kde.org/show_bug.cgi?id=303830 REPOSITORY R294 KBookmarks REVISION DETAIL https://phabricator.kde.org/D6277 AFFECTED FILES src/kbookmarkmanager.cpp src/kbookmarkmanager.h To: valeriymalov, #frameworks
