D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

pino added a comment. In D29299#676448 , @dfaure wrote: > When you wrote "ki18n_install() is basically used by KF sources that use ECM already" it seemed to me that this was looking at KDE community code only FWIW, I also checked

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

pino added a comment. In D29299#676446 , @dfaure wrote: > In D29299#676445 , @pino wrote: > > > I asked for actual **valid** use cases when using the new variables first would break, and I still got

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

pino added a comment. In D29299#676439 , @dfaure wrote: > @pino Other than the fact that you think D29136 is "good enough", do you have any concrete objection to this version? I think I already wrote

D27633: Port to KPluginLoader

pino added a comment. In D27633#619369 , @aacid wrote: > In D27633#619365 , @aacid wrote: > > > I think this broke

D29805: Thumbnail djvu: Avoid a crash when djvu is not installed

pino added a comment. In D29805#674218 , @meven wrote: > In D29805#674206 , @pino wrote: > > > In D29805#674205 , @meven wrote: > > > > > In

D29805: Thumbnail djvu: Avoid a crash when djvu is not installed

pino added a comment. In D29805#674205 , @meven wrote: > In D29805#674204 , @pino wrote: > > > > FIXED-IN: 20.08 > > > > still for 20.08... > > > Yes kio-extra is released with KDE

D29805: Thumbnail djvu: Avoid a crash when djvu is not installed

pino added a comment. > FIXED-IN: 20.08 still for 20.08... REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D29805 To: meven, #frameworks, broulik, ngraham, pino Cc: pino, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov, iasensio,

D29805: Thumbnail djvu: Avoid a crash when djvu is not installed

pino added inline comments. INLINE COMMENTS > meven wrote in djvucreator.cpp:52-54 > I'd like to use instead the Framework coding style to improve homogenizing > coding styles. > https://community.kde.org/Policies/Frameworks_Coding_Style#Braces this code does not follow that style, so please

D29805: Thumbnail djvu: Avoid a crash when djvu is not installed

pino added a comment. please target `release/20.04` for this fix, thanks INLINE COMMENTS > djvucreator.cpp:52-54 > + if (QStandardPaths::findExecutable(QStringLiteral("ddjvu")).isEmpty()) { > + return false; > + } extra brackets REPOSITORY R320 KIO Extras REVISION DETAIL

D28673: [PackageUrlInterceptor] Make QRegularExpression static

pino added a comment. In D28673#672845 , @broulik wrote: > so, can the regexp be replaced or does it need to stay? Sure: look for `/ui/` in path, and if it is there, join what's before it + prefix + what's after it. REPOSITORY R242

D29805: Thumbnail djvu: avoid to exit(1) when it should not

pino requested changes to this revision. pino added a comment. This revision now requires changes to proceed. Please update the text of the revision and the commit message, as they don't make much sense. Please mention that it fixes a crash in case ddjvu is not installed, by avoiding

D29805: Thumbnail djvu: avoid to exit(1) when it should not

pino requested changes to this revision. pino added a comment. This revision now requires changes to proceed. This change is definitely not correct. > A child process exited with 1 as result but it wasn't an error case. it is an error case: the execv*() family [1] of functions

D29381: Thumbnail text: use libmagic to detect encoding

pino added a comment. why not KEncodingProber from the KCodecs framework, like also the commented bits hint about? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D29381 To: meven, #frameworks, sitter, ngraham Cc: pino, kde-frameworks-devel, kfm-devel, azyx,

D29254: [RenameDialog] Add an arrow indicating direction from src to dest

pino added inline comments. INLINE COMMENTS > ahmadsamir wrote in renamedialog.cpp:299 > @pino, right; dolphin isn't rtl-aware (or if it is, I couldn't find out how > to switch it to rtl). But I agree the code here should account for rtl anyway. > > @ngraham: I think we should stick to the

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

pino added a comment. In D29299#660614 , @kossebau wrote: > In D29299#660519 , @pino wrote: > > > In D29299#660505 , @kossebau wrote: > > > > > Because

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

pino added a comment. In D29299#660505 , @kossebau wrote: > Because people do strange things, and I prefer to rather not break their card house unless necessary. Again, in which cases? The only way it might change the path is: use ki18n

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

pino added a comment. In D29299#660490 , @kossebau wrote: > D29136 in the current version though changes behaviour by favouring KDE_INSTALL_LOCALEDIR over LOCALE_INSTALL_DIR. Which at least in theory might

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

pino added a comment. In D29299#660466 , @kossebau wrote: > In D29299#660465 , @pino wrote: > > > Also, your patch basically includes D29136 in the case of no

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

pino added a comment. > The patch should not require existing users to adapt Yes, that's also what I wrote earlier. Also, your patch basically includes D29136 in the case of no DESTINATION parameter specified, hence my suggestion is: - edit

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

pino added a comment. > The problem is that ki18n_install() is rarely used manually OK, at least from LXR search it seems a bit more than that: KF packages using ki18n, some extragear stuff, and few Plasma bits. This means that KF packages can switch to that immediately (because of the

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

pino added a comment. The problem is that ki18n_install() is rarely used manually, and generally it is appended by the release scripts to the top-level CMakeLists.txt file that goes into the tarballs. This means that either the majority of the packages will not make use of this, or a

D29254: [RenameDialog] Add an arrow indicating direction from src to dest

pino added inline comments. INLINE COMMENTS > renamedialog.cpp:299 > +// direction from src to dest > +const QPixmap pix = > QIcon::fromTheme(QStringLiteral("go-next")).pixmap(d->m_srcPreview->height()); > +srcToDestArrow->setPixmap(pix); this is not right-to-left

D29063: Fix testpackage-appstream: XDG_DATA_DIRS needs to be explicitly extended

pino added a comment. (Commented in the wrong place) Shouldn't the test completely ignore the system location, to avoid interferences from the system installation? REPOSITORY R290 KPackage REVISION DETAIL https://phabricator.kde.org/D29063 To: kossebau, #frameworks, mart, apol,

D28755: Breeze Icons cannot be built from read-only source location

pino added a comment. Fixed with def21bf6c691a40bef233efd898fcb7ff57d55bb , thanks for the notice. REPOSITORY R266 Breeze Icons REVISION DETAIL https://phabricator.kde.org/D28755 To: marten, #breeze, ngraham

D28760: KSettings::Dialog: avoid duplicate entries due cascading $XDG_DATA_DIRS

pino added a comment. Why not instead use a `QMap` to collect the files? This way you wouldn't need the double QStandardPaths lookup. REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D28760 To: dfaure, apol, broulik, davidedmundson, kossebau Cc: pino,

D28755: Breeze Icons cannot be built from read-only source location

pino added a comment. Or maybe the other way round: - add an optional parameter to the script to specify the source directory, defaulting to "." - run the script in the build directory, passing the source directory as parameter REPOSITORY R266 Breeze Icons REVISION DETAIL

D28673: [PackageUrlInterceptor] Make QRegularExpression static

pino added a comment. Does it really need to be a regular expression, though? A simple string search & replace should do the same thing. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28673 To: broulik, #plasma Cc: pino, kde-frameworks-devel,

D28532: Introduce more user-visible error reporting for installations

pino added a comment. Please fix the i18n() calls, as the values of placeholders are passed as parameter to it instead of using .arg(): i18n("foo %1").arg(foo) // WRONG i18n("foo %1", foo) // correct REPOSITORY R304 KNewStuff REVISION DETAIL

D28397: Replace Vokoscreen with VokoscreenNG

pino added a comment. please do //not// change translated keys (eg `Name[lang]`) in desktop-alike files when you change the English string: we have a system in place to handle translations REPOSITORY R304 KNewStuff BRANCH update-vokoscreen-url (branched from master) REVISION DETAIL

D28355: Introduce function ecm_install_configured_file

pino added inline comments. INLINE COMMENTS > davidedmundson wrote in ECMConfiguredInstall.cmake:46-48 > Maybe. > > My rationale for not forcing a suffix is sometimes we do this configure dance > for .desktop files and there we have to be a bit careful with scripty. > > But generally it's

D28355: Introduce function ecm_install_configured_file

pino added a comment. Nice one! I cannot test right now though, I might do it over the weekend (do not hold on me though). I took the liberty of doing some formatting changes to the header of the new file, what do you think? #.rst: # ECMConfiguredInstall #

D28324: [Inotify] Remove dead/duplicate code

pino added inline comments. INLINE COMMENTS > bruns wrote in kinotify.cpp:350 > asserts are IMHO pointless, nobody enables them, but it clutters the code. > Anyone touching the code should and has to read the man pages etc anyway. no, an assert is sort of "program by contract": you make it

D28355: Introduce function ecm_install_configured_file

pino added a comment. Can you please adapt it so `_template` can be an absolute path? REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D28355 To: davidedmundson Cc: pino, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, michaelh,

D28324: [Inotify] Remove dead/duplicate code

pino added inline comments. INLINE COMMENTS > bruns wrote in kinotify.cpp:350 > man inotify > > > IN_Q_OVERFLOW > > Event queue overflowed (wd is -1 for this event). then add an assert for this case, referencing the documentation? REPOSITORY R293 Baloo REVISION DETAIL

D28324: [Inotify] Remove dead/duplicate code

pino added inline comments. INLINE COMMENTS > bruns wrote in kinotify.cpp:350 > First check is here ... but only also if wd is < 0 REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D28324 To: bruns, #baloo, ngraham Cc: pino, kde-frameworks-devel, hurikhan77, lots0logs,

D17816: Support for xattrs on kio copy/move

pino added a comment. In D17816#627921 , @cochise wrote: > In D17816#627918 , @bruns wrote: > > > Not true in general ... please add back, and **check** if the destination FS supports XAttrs. If

D26358: KIO/SMB convert kio protocol declaration to json format

pino added inline comments. INLINE COMMENTS > smb.json:27 > +"output": "filesystem", > +"protocol": "recentlyused", > +"reading": true, erm... > smb.protocol:11 > -deleting=true > -opening=true > -moving=true what about this key? REPOSITORY R320 KIO

D26410: Port KLanguageButton from KConfig to QSettings

pino added a comment. IIRC `KConfigGroup::readEntry() reads the translated version of the key, and every `kf5_entry.desktop` has such translations. QSettings does not do that. Did you test only in an English setup? REPOSITORY R265 KConfigWidgets BRANCH klb REVISION DETAIL

D26273: cmake: don't use taglib-config if we are cross compiling

pino added a comment. Or maybe switch this module to use pkg-config as primary way to detect taglib, falling back to taglib-config only when the pkg-config file is not available. REPOSITORY R286 KFileMetaData BRANCH bshah/cross REVISION DETAIL https://phabricator.kde.org/D26273 To:

D24932: Add button to open the folder in filelight to view more details

pino added inline comments. INLINE COMMENTS > kpropertiesdialog.cpp:1453 > +// Open the current folder in filelight > +KRun::run(QStringLiteral("/usr/bin/filelight"), { properties->url() }, > properties->window(), QStringLiteral("Filelight"), > QStringLiteral("filelight")); > +}

D24476: [KPropertiesDialog] Only show volume-related info for volumes

pino added a comment. In D24476#543298 , @ngraham wrote: > In D24476#543297 , @broulik wrote: > > > Perhaps, but right clicking on my HOME will then give me no information as it's not a separate

D7446: [Places panel] Revamp the Recently Saved section

pino added inline comments. INLINE COMMENTS > kfileplacesitem.cpp:113 > case KFilePlacesModel::RecentlySavedType: > -m_groupName = i18nc("@item", "Recently Saved"); > +m_groupName = i18nc("@item", "Recent"); > break; this string needs a context, as "recent"

D23579: WIP: port ftp slave to new error reporting system

pino added inline comments. INLINE COMMENTS > anthonyfieroni wrote in ftp.h:49 > That's BIC we cannot remove, reorder nor insert parent(s) > That's BIC we cannot remove, reorder nor insert parent(s) This is a kioslave, built as plugin. There is no such thing as BC for this kind of stuff.

D23815: [KConfig] port away from deprecated methods in Qt 5.14

pino added a comment. In D23815#528543 , @dfaure wrote: > If we want to enable this flag (to "punish" the first KDE developer who upgrades Qt, like me currently), then at least we should only do so in git checkouts, not in release tarballs. >

D23667: Add == and != operators to KIO::UDSEntry

pino added a comment. please use `QCOMPARE`/`QVERIFY` instead of `Q_ASSERT` in QTest tests INLINE COMMENTS > udsentrytest.cpp:301 > + > +// 4th entry : an additionnal field > +KIO::UDSEntry entry4; typo, "additional" REPOSITORY R241 KIO BRANCH master REVISION DETAIL

D23667: Add == and != operators to KIO::UDSEntry

pino added a comment. Oh, and also please add tests for them in UDSEntryTest. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D23667 To: meven, #frameworks Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

pino added a comment. Please move the implementations in the cpp file, otherwise it will be impossible to change/fix the implementation later on in a binary compatible way. Also: - both the operators ought to be const, since they do not mutate the object (and otherwise they cannot

D22510: Added dialog to set execute permission for executable file when trying to run it.

pino added inline comments. INLINE COMMENTS > krun.cpp:164 > > +bool KRun::hasExecuteBit(const QString ) > +{ please make this function as file static, so there is no need to expose it as API of the KRun class > krun.cpp:310 > +// Helper function that attempts to set execute bit for given

D23470: Offer an xdg-compatible mode for convertToQVariant

pino added inline comments. INLINE COMMENTS > kconfiggroup.h:655 > + > +KCONFIGCORE_DEPRECATED_EXPORT static QVariant convertToQVariant(const > char *pKey, const QByteArray , const QVariant ) { return > convertToQVariant(pKey, value, aDefault, false); } > friend class KServicePrivate;

D23224: Fix capitalization of Nextcloud

pino added a comment. In D23224#513556 , @z3ntu wrote: > I'm guessing the changed English strings will trigger a "untranslated string" warning in the translation software, so the translators should notice it, right? Yes. REPOSITORY

D23224: Fix capitalization of Nextcloud

pino added a comment. Please do **not** change the translations manually -- there is an automatic system that takes care of extracting the English strings, and merging back the translators for desktop/json/xml/alike files. If the English text is correct but a translation is not: please

D22836: Fix checking dirs for metainfo.yaml with non-ascii chars with Python 2.7

pino added a comment. In D22836#504394 , @davidre wrote: > In D22836#504355 , @kossebau wrote: > > > That as well, as also discussed on irc today. > > > > Just in case that other scripts on

D22836: Fix checking dirs for metainfo.yaml with non-ascii chars with Python 2.7

pino added a comment. In D22836#504393 , @aacid wrote: > Ok, yep i may have misunderstood :) > > But if you say "kapidox supports Python 2.7" we need to commit this to keep it true :) See my very first comment. REPOSITORY R264

D22836: Fix checking dirs for metainfo.yaml with non-ascii chars with Python 2.7

pino added a comment. In D22836#504389 , @aacid wrote: > Pino please take a step back and rethink your position. > Yes, supporting Python2 sucks, but this is **2** lines of code while porting everything to Python 3 is lots of work more.

D22836: Fix checking dirs for metainfo.yaml with non-ascii chars with Python 2.7

pino added a comment. Let me add one more thing: I understand you care about API docs, about making them available for our users on api.kde.org, and about giving the best impression possible about them (I remember various patches about that in the past). I just don't agree with the

D22836: Fix checking dirs for metainfo.yaml with non-ascii chars with Python 2.7

pino added a comment. In D22836#504359 , @kossebau wrote: > In D22836#504358 , @pino wrote: > > > Oh dear, and what about switching the api generation to Python 3 instead? > > > Nobody is

D22836: Fix checking dirs for metainfo.yaml with non-ascii chars with Python 2.7

pino added a comment. In D22836#504357 , @kossebau wrote: > In D22836#504356 , @pino wrote: > > > In D22836#504355 , @kossebau wrote: > > > > > Just in

D22836: Fix checking dirs for metainfo.yaml with non-ascii chars with Python 2.7

pino added a comment. In D22836#504355 , @kossebau wrote: > Just in case that other scripts on api.kde.org do not work yet with Python3 or no-one has time to test that, At this point, making Python stuff work with Python3 is priority

D22836: Fix checking dirs for metainfo.yaml with non-ascii chars with Python 2.7

pino added a comment. Or maybe just drop support for Python 2 entirely? - kapidox is rarely used - Python 2 will be EOL in 5 months - distros either switched kapidox to Python 3, or will do it soon REPOSITORY R264 KApiDox REVISION DETAIL https://phabricator.kde.org/D22836 To:

D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

pino requested changes to this revision. pino added a comment. This revision now requires changes to proceed. other than building, please also check that it actually still works on Linux INLINE COMMENTS > kio_sftp.cpp:383 > fileType = QT_STAT_MASK - 1; > -#ifdef Q_OS_WIN > -

D22606: add missing semicolon

pino added a comment. Patch landed, and build successfully everywhere. Congratulations for your first patch :) REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D22606 To: antonioh, pino Cc: pino, kde-frameworks-devel, #baloo, LeGast00n, sbergeron,

D22606: add missing semicolon

pino closed this revision. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D22606 To: antonioh, pino Cc: pino, kde-frameworks-devel, #baloo, LeGast00n, sbergeron, fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D22606: add missing semicolon

pino accepted this revision. pino added a comment. This revision is now accepted and ready to land. Do you have permissions to push patches on your own? If not, please specify your email so I can properly attribute this patch to you. REPOSITORY R286 KFileMetaData REVISION DETAIL

D22599: Make first and last name available separately

pino added a comment. How would first + last name work, when a) you have multiple first names b) you have multiple last names c) you have only one "name" -- http://wookware.org/name.html REPOSITORY R307 KPeople BRANCH master REVISION DETAIL https://phabricator.kde.org/D22599

D22105: WIP : Fix SFTP Plugin of KIO for Windows

pino added inline comments. INLINE COMMENTS > kio_sftp.cpp:2047 > +QString error_msg = receivedFile.errorString(); > +qDebug() << QStringLiteral("Couldn't update modified > time : ") << error_msg; > +} no need for QStringLiteral here,

D22102: Implement apply-on-double-click for all grid view KCM delegates

pino added a comment. In D22102#488447 , @ngraham wrote: > In D22102#488388 , @pino wrote: > > > While I can understand that clicking can be "annoying", OTOH I consider all the KCMs you mentioned as

D22102: Implement apply-on-double-click for all grid view KCM delegates

pino added a comment. In D22102#487806 , @ngraham wrote: > Right now, I find it *extremely* annoying, slow, and frustrating to test new themes, colors, icons, wallpapers, etc. The workflow is to click on the delegate, and then click on the

D22102: Implement apply-on-double-click for all grid view KCM delegates

pino added a comment. In D22102#486968 , @davidedmundson wrote: > From the old review: > > pino: > > -1 for this: > > hidden feature > it does not make sense when the general paradigm is OK/Apply/Cancel, so we are

D22047: Fix translation of actiondisplay

pino added a comment. I do not see how this is supposed to fix the translations. Nothing translates the strings statically (like the various `Name`/`Comment`/etc), nor at runtime. REPOSITORY R495 Purpose Library REVISION DETAIL https://phabricator.kde.org/D22047 To: nicolasfella, apol,

D21721: [WIP] Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

pino added inline comments. INLINE COMMENTS > engine.h:471 > + */ > +virtual CommentsModel* commentsForEntry(const KNSCore::EntryInternal > ); > + BIC change, you cannot add virtual functions in a public class > provider.h:148 > virtual void loadPayloadLink(const EntryInternal ,

D21661: add snoretoast backend for KNotifications on Windows

pino added a comment. Ah, one last thing I apparently forgot to mention so far: the names of class variables must start with `m_`. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D21661 To: brute4s99, broulik, sredman, vonreth, albertvaka Cc: nicolasfella,

D21661: add snoretoast backend for KNotifications on Windows

pino added a comment. In D21661#479366 , @brute4s99 wrote: > I think I should make a new diff for further discussions, as this one is quite riddled with suggestions now. Are there any more issues with this patch or should I continue with a new

D21661: add snoretoast backend for KNotifications on Windows

pino added a comment. In D21661#476607 , @sredman wrote: > In D21661#476571 , @pino wrote: > > > It seems the snoretoast library provides a `SnoreToasts` class to do this instead of spawning an

D21661: add snoretoast backend for KNotifications on Windows

pino added a comment. > This comment was removed by brute4s99. This comment was: > updated wrt review by toscanos Please do **NOT** remove your own comments. Other than being anti-social (imagine how easily you can twist conversations), in this case it's perfectly useless, as

D21661: add snoretoast backend for KNotifications on Windows

pino added inline comments. INLINE COMMENTS > CMakeLists.txt:112 > +if (TARGET SnoreToast::SnoreToastActions) > + target_link_libraries(KF5Notifications PRIVATE Qt5::Core Qt5::Network > SnoreToast::SnoreToastActions) > +endif () `Qt5::Core` is not needed here; if we really want to be

D21661: add snoretoast backend for KNotifications on Windows

pino added a comment. In D21661#476156 , @pino wrote: > how is snoretoast actually used here? you are requiring the library for building and linking, but then: > > - `snoretoastactions.h`, which is part of the headers of snoretoast, is

D21661: add snoretoast backend for KNotifications on Windows

pino added inline comments. INLINE COMMENTS > notifybysnore.cpp:115 > +{ > +if (m_notifications.find(notification->id()) != m_notifications.end()) { > +qCDebug(LOG_KNOTIFICATIONS) << "Duplicate notification with ID: > " << notification->id() << " ignored."; since this method is

D21661: add snoretoast backend for KNotifications on Windows

pino added a comment. how is snoretoast actually used here? you are requiring the library for building and linking, but then: - `snoretoastactions.h`, which is part of the headers of snoretoast, is copied here - the snoretoast library is never used, as the utilities of it are invoked

D21298: Add example app for printing highlighted text to pdf

pino added inline comments. INLINE COMMENTS > main.cpp:36 > +parser.addPositionalArgument(QStringLiteral("source"), > QStringLiteral("The source file to print.")); > +parser.addPositionalArgument(QStringLiteral("pdf"), QStringLiteral("The > PDF file to prin to.")); > +

D21146: KProcessInfoList -- add proclist backend for FreeBSD

pino added a comment. In D21146#463948 , @tcberner wrote: > Gargh, the code `kdelibs` code formatter changed a lot more in `kprocesslist_unix_proc.cpp` than wanted :/ Then just rename kprocesslist_unix.cpp to kprocesslist_unix_proc.cpp,

D21146: KProcessInfoList -- add proclist backend for FreeBSD

pino added a comment. What about create a separate kprocesslist_libprocstat.cpp (or so) to implement `KProcessList::processInfoList()` using libprocstat, instead of overloading the existing kprocesslist_unix.cpp? Also, please adjust the coding style to the kdelibs coding style:

D17991: Refactor the way device backends are built and registered

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit R245:97d95e3171c3: Refactor the way device backends are built and registered (authored by pino). CHANGED PRIOR TO COMMIT

D20983: Exclude .gcode and .vdi files from indexing consideration

pino added a comment. While you are there adding filters, what about: - `*.qcow2` -- QCOW2 disk images for QEMU - `*.raw`/`*.img` -- typical extensions for raw disk images (there is no fixed extension, as a raw disk image is just a file with bit-by-bit representation of the disk of a

D20964: [FileWidget] Replace "Filter:" with "File type:" when saving with a limited list of mimetypes

pino added inline comments. INLINE COMMENTS > kfilewidget.cpp:2804-2806 > +if (operationMode == KFileWidget::Saving && > filterWidget->isMimeFilter()) { > +label = i18n(" type:"); > +whatsThisText = i18n("This is the file type selector. It is used > to select the format

D20735: [KPropertiesDialog] Add octal permissions

pino requested changes to this revision. pino added a comment. This revision now requires changes to proceed. The permission changes to ktelnetservice5.desktop are unrelated, please revert them. Mostly important, the value shown in the advanced permissions dialog is not taking into

D20735: [KPropertiesDialog] Add octal permissions

pino requested changes to this revision. pino added a comment. Also, there was feedback it was still not taken care. Please do not ping on pathed when you are requested for changes, and still do not do them. INLINE COMMENTS > kpropertiesdialog.cpp:2111 > +mode_t itemPermission =

D20735: [KPropertiesDialog] Add octal permissions

pino added a comment. Also: what is the use case of this feature? For casual users the octal permissions make no sense, they are mostly useful for power users (if they don't even just use the terminal for these things). At most, this would fit in the "advanced permissions" dialog.

D20735: [KPropertiesDialog] Add octal permissions

pino added a comment. In D20735#454271 , @shubham wrote: > How to print it as octal? http://lmgtfy.com/?q=QString+arg REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D20735 To: shubham, ngraham, pino Cc: pino,

D20735: [KPropertiesDialog] Add octal permissions

pino added a comment. In D20735#454260 , @shubham wrote: > I had used permission() to get the mode_t variable, which I later type casted to qint64, still the permissions resulted were completed wrong. Tried to print it as octal-based

D20735: [KPropertiesDialog] Add octal permissions

pino requested changes to this revision. pino added a comment. This revision now requires changes to proceed. Parsing the result of KFileItem::permissionsString() is a rather bad idea, especially considering that KFileItem already provides mode() and permissions(). Also, the placing of

D20169: Add profile support interface for TerminalInterface

pino added a comment. General notes on the API (not a konsole developer though): - an abstract virtual destructor is needed - in kdelibs/kf5 lingo, the "get" as prefix of API getters is generally not used - `setCurrentProfile` instead of `changeCurrentProfile`? the latter makes me

D20169: Add profile support interface for TerminalInterface

pino added a comment. Yes, this change breaks the binary compatibility. Please add a TerminalInterfaceV2 that inherits TerminalInterface instead, or add a separate interface for profile handling. REPOSITORY R306 KParts REVISION DETAIL https://phabricator.kde.org/D20169 To: mschiller,

D20626: Refactor and cleanup

pino added a comment. Also, please explicitly mention what are the changes done. "refactor and cleanup" is very vague, while saying that, for example, QSysInfo is used on all the OSes is better. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D20626 To: shubham, dfaure

D20506: KCharSelect's internal model: ensure rowCount() is 0 for valid indexes

pino added a comment. Shouldn't the same applied to `columnCount` too? REPOSITORY R236 KWidgetsAddons BRANCH master REVISION DETAIL https://phabricator.kde.org/D20506 To: dfaure, cfeck Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D20096: Fill UDSEntry::UDS_CREATION_TIME under linux when glibc >= 2.28

pino added inline comments. INLINE COMMENTS > meven wrote in file.cpp:850-870 > Unfortunately this is not possible here : statx is also a function, the > compiler gets messed up when removing the struct keyword interpreting it as a > function call. > > > /file/file.cpp:850:34: warning: inline

D20096: Fill UDSEntry::UDS_CREATION_TIME under linux when glibc >= 2.28

pino added inline comments. INLINE COMMENTS > pino wrote in ConfigureChecks.cmake:19 > just do the test unconditionally sorry, i realized that my sentence was ambiguous: i meant to check for the existance of statx no matter the platform REPOSITORY R241 KIO BRANCH arcpatch-D20096

D20096: Fill UDSEntry::UDS_CREATION_TIME under linux when glibc >= 2.28

pino added inline comments. INLINE COMMENTS > ConfigureChecks.cmake:19 > + > +if (LIBC_IS_GLIBC) > +check_cxx_source_compiles(" just do the test unconditionally > ConfigureChecks.cmake:24 > +int main() { > + int t = STATX_BASIC_STATS; > + return 0; other than

D20096: Fill UDSEntry::UDS_CREATION_TIME under linux when glibc >= 2.28

pino added inline comments. INLINE COMMENTS > file.cpp:72-74 > +#if defined(STATX_BASIC_STATS) && defined(__GLIBC__) > +#define USE_STATX > +#endif TBH, instead of this static define, I'd do a proper cmake check (see ConfigureChecks.cmake, and config-kioslave-file.h.cmake in

D17991: Refactor the way device backends are built and registered

pino added a comment. BTW if nobody will comment on this within one month, I will commit it as-is. 4 months without a review will be enough, considering also other reviews for Solid got comments/approvals. REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D17991

D17991: Refactor the way device backends are built and registered

pino updated this revision to Diff 55373. pino added a comment. Rebase on master. REPOSITORY R245 Solid CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17991?vs=51892=55373 BRANCH cmake-backends-refactor (branched from master) REVISION DETAIL

  1   2   3   >