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
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
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
valeriymalov marked 2 inline comments as done.
REPOSITORY
R294 KBookmarks
REVISION DETAIL
https://phabricator.kde.org/D6277
To: valeriymalov, #frameworks
Cc: aacid, ltoscano
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,
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
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);
> +
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"
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
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
> +
valeriymalov edited the summary of this revision.
REPOSITORY
R294 KBookmarks
REVISION DETAIL
https://phabricator.kde.org/D6277
To: valeriymalov, #frameworks
Cc: ltoscano
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 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
13 matches
Mail list logo
