- User Since
- Mar 5 2015, 12:44 PM (107 w, 2 d)
Looks good to me. Just some minor things I noticed.
Thu, Mar 23
Ah yes OK, the extraction on Linux is indeed not done in those Qt patches either. Nevermind ;)
The comment about ctime is still true ;-)
Feel like implementing it for Linux, too? ;-)
Wed, Mar 22
Not right now, see kxmlgui/src/kshortcutseditoritem.cpp:54
m_actionNameInTable = i18nc([...] KLocalizedString::removeAcceleratorMarker(m_action->text()));
which is returned further down as DisplayRole for the Name column.
But you could easily add support for a custom property here ("descriptiveText") and document that, it would certainly be useful for all those actions whose text only makes sense in the context of the submenu they're in.
Oh, yeah, that sounds backwards.
createGUI(part) takes care of loading ui_standards.rc, see the bit of code I pointed to in my last comment.
This looks like ui_standards.rc isn't loaded.
Does this code call createGUI(part) anywhere? Do you get into KParts::MainWindow::createShellGUI(), and does it end up calling setXMLFile(KXMLGUIClient::standardsXmlFileLocation()); in kparts/src/mainwindow.cpp:168 ?
Tue, Mar 21
Sun, Mar 19
Bug in the commit log, Chrome 57, not Chromium 52 ;)
Sat, Mar 18
Wed, Mar 15
I should have said this earlier, but qobject_cast<QDialog *>(o) would even be better than inherits, and the isWidgetType() check is unnecessary anyway (a dialog is a widget, yes). Oh well ;) Either way, ship it.
Well, KDialog inherits from QDialog, so your new check covers both.
Would be worth doing a search for all uses of KUser::homeDir() then -- and possibly deprecated it, except if it makes sense for e.g. a user-management KCM?
Mon, Mar 13
Sun, Mar 12
Ship it ;)
Yes; looks much nicer, doesn't it?
Sat, Mar 11
Extra credit for factorizing this into a file-static function and handling data: URLs separately (no point in seeing 100 chars of data crap, we could do data:[...] or something like that).
Can you use KStringHandler::csqueeze ? This way filenames are still visible.
Fri, Mar 10
Thu, Mar 9
Wed, Mar 8
"I didn't find signals to notify changes" --> you mean how to tell apps that something changed in a given protocol? That's what KDirNotify is for, but you can't do the monitoring from a short-lived slave, you need something that stays alive for the whole session, and that's what a kded module can be used for.
BTW don't port the callers to this new method yet, we'll have to wait until the KF5 minimum requirement is raised in kdialog and plasma-integration. I made a TODO for June ;)
Add test for relative URL and document the proper way to do it
Can you use arc diff to upload changes? Then context would be available in the diffs (I think).
I don't know the full architecture of this kauth stuff, but isn't it possible to save to <destinationfile>.tmp rather than /tmp, ~/.cache or anywhere else? Then instead of "more likely" we are *sure* it's on the same device as the destination.
Tue, Mar 7
I thought about it, but the call sites I found didn't actually need that. So unless you know of one, I changed my mind to "let's wait for someone to request it". Although I guess it won't happen because people will just pass a QUrl instead ;-)
I just found that this bug was very likely introduced by the fix for https://bugs.kde.org/show_bug.cgi?id=369216, proving that a method that takes a badly defined QString leads to some code (kdialog) calling it with a path and other code (filedialog integration plugin) calling it with URLs, which only leads to ping-pong fixes ;)
KFileItem::localPath() should not ever ever be a URL like desktop:/. That would be very wrong.
It just returns what you set in UDS_LOCAL_PATH, so it's kio_desktop you should debug, not kio ;)
Well except for KIO::ForwardingSlaveBase, called by kio_desktop -- note that ForwardingSlaveBase::prepareUDSEntry already sets UDS_LOCAL_PATH, but ok you call insert again which replaces that value.
Right the only way to get atomic renaming is to write the tempfile next to its final destination, NOT in /tmp.
(just like QSaveFile does ;)
Mon, Mar 6
Sun, Mar 5
If you say it works... ;)
Sat, Mar 4
I'm curious to find out whether the clang static analyzer still complains or realizes this is fine now :)
Seems like a valid fix.
Fri, Mar 3
QDir::cleanPath or QUrl::NormalizePathSegments can help making this fix more generic.
I think you can remove the static lib, yes. It didn't "look" like a proper KF5 lib, being the only static one, and the functions being undocumented, etc.
Looks good - just a minor nitpick on coding style / readability, feel free to push (either way, actually).
Thu, Mar 2
Tue, Feb 28
Yes interface->isServiceRegistered(dbusServiceName) is technically blocking, but it can't block for a long time, since it's only talking to the dbus daemon. The reply comes in rather quickly, unlike a blocking call to another KDE process which could be busy or where the call itself could take a long time to be processed.
In summary, I don't think an existing call to isServiceRegistered can be used as an argument for making (potentially much much longer) blocking calls.