Review Request 126610: kwidgetitemdelegate: properly cleanup widgets on index removal

iff: https://git.reviewboard.kde.org/r/126610/diff/ Testing --- A sample application with widgets for items in the model, removing indexes: no more warning at removal time. Thanks, Pino Toscano ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde

Re: Review Request 120139: kill warning

better debugged why it is triggered. Anyway, I just posted https://git.reviewboard.kde.org/r/126610/ to avoid triggering it on manual index removal. - Pino Toscano On Set. 11, 2014, 10:48 a.m., Sune Vuorela wrote: > > --- > T

Re: Review Request 126610: kwidgetitemdelegate: properly cleanup widgets on index removal

Gen. 2, 2016, 7:24 p.m., Pino Toscano wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126610/ > --- >

Re: Menu/Command capitalization

t other OSes. At least for me, it has been hard enough to keep KDE applications following our style in the last 10+ years, I don't want to start over again. -- Pino Toscano signature.asc Description: This is a digitally signed message part. ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Menu/Command capitalization

In data venerdì 8 gennaio 2016 12:51:35, Jaroslaw Staniek ha scritto: > On 8 January 2016 at 00:28, Pino Toscano wrote: > > Hi, > > > > In data giovedì 7 gennaio 2016 22:42:18, Jaroslaw Staniek ha scritto: > >> Not too long ago MS Windows has moved from "Titl

Review Request 126936: help: fix garbage sent when serving static files

end. Thanks, Pino Toscano ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126936: help: fix garbage sent when serving static files

marked as submitted. Review request for KDE Frameworks, David Faure and Luigi Toscano. Changes --- Submitted with commit 3642b5284e65d94db22a86d52e710dd60e15b230 by Pino Toscano to branch master. Repository: kio Description --- Switch from a `QByteArray` to a simplier stack-based

Review Request 127002: Add KLocalizedString::getLanguages()

https://git.reviewboard.kde.org/r/127002/diff/ Testing --- Builds, tests pass. Thanks, Pino Toscano ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127002: Add KLocalizedString::getLanguages()

marked as submitted. Review request for KDE Frameworks and Chusslove Illich. Changes --- Submitted with commit 84359916472cf872271703272d87bfe73a4fa69b by Pino Toscano to branch master. Repository: ki18n Description --- Easy way to get the ordered list of languages where

Review Request 127106: applications.menu: remove references to unused categories

` -- replaced by KidsGame Nothing on kde.org references them, and neither anything currently packaged in Debian. Diffs - src/applications.menu f2d52538c4ed6dc39d04a10e7c072cb060b3fd2b Diff: https://git.reviewboard.kde.org/r/127106/diff/ Testing --- Thanks, Pino Toscano

Re: Review Request 126610: kwidgetitemdelegate: properly cleanup widgets on index removal

--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126610/#review94071 --- ping - Pino Toscano On Mar. 28, 2016, 1:39 p.m., Pino

Review Request 127972: Always update the Predicate parser from y/l sources

flex 2.6.0 and bison 3.0.4; `make test` passes too. Thanks, Pino Toscano ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127972: Always update the Predicate parser from y/l sources

------- On May 19, 2016, 10:19 p.m., Pino Toscano wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127972/ > -

Re: Review Request 127972: Always update the Predicate parser from y/l sources

--------- On May 20, 2016, 6:39 a.m., Pino Toscano wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127972/ >

Re: Review Request 127972: Always update the Predicate parser from y/l sources

site-ports_kf5_KF5-Frameworks/kf5-solid/work/build' > > make: *** [all] Error 2 > > make: Leaving directory > > `/opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/build' > > ``` > > René J.V. Bertin wrote:

Re: Review Request 127972: Always update the Predicate parser from y/l sources

ckly). - Pino --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127972/#review95658 --- On May 20, 2016, 6:39 a.m., Pin

Re: Review Request 126610: kwidgetitemdelegate: properly cleanup widgets on index removal

ociated widgets. - Pino --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126610/#review94076 --- On March 28, 2016, 1:41

Re: Review Request 127972: Always update the Predicate parser from y/l sources

g directory > > `/opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/build' > > make[1]: *** [src/solid/CMakeFiles/KF5Solid_static.dir/all] Error 2 > > make[1]: Leaving directory > > `/opt/local/var/macports/build/_opt_local_site-ports_kf

Review Request 127984: Always update the Trader parser from y/l sources

. Thanks, Pino Toscano ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 123811: Use tcgetattr & tcsetattr if available

d test above should make sure there's no build behaviour change if a platform does not have those functions. Thanks, Pino Toscano ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123811: Use tcgetattr & tcsetattr if available

`ioctl` code is used as before, build is fine and so tests too Not tested on other OSes, but the second test above should make sure there's no build behaviour change if a platform does not have those functions. Thanks, Pino Toscano ___ Kde-fra

Re: Review Request 123811: Use tcgetattr & tcsetattr if available

marked as submitted. Review request for KDE Frameworks and Oswald Buddenhagen. Changes --- Submitted with commit 35ea45b588db9afcbd796576833ac338c6b4b8e8 by Pino Toscano to branch master. Repository: kpty Description --- Look for `tcgetattr` & `tcsetattr`, and use them if f

Re: Review Request 127833: KWallet: More Coverity fixes, and include Qt headers for endianness check.

> On May 23, 2016, 11:20 a.m., David Faure wrote: > > Please note that: > > * the Q_BYTE_ORDER change was reverted, since it made the wallet storage > > incompatible with previous releases. This needs further analysis in order > > to improve the code while retaining compat. > > * kwallet is def

Re: Review Request 127833: KWallet: More Coverity fixes, and include Qt headers for endianness check.

llets which would then not match > with KWallet when it was fixed. > > Either way someone's systems are being broken here, because the current > code unilaterally enables bit-shuffling always no matter if the CPU is > big-endian or little-endian. But I don't have a b

Re: Review Request 127972: Always update the Predicate parser from y/l sources

marked as submitted. Review request for KDE Software on Mac OS X, KDE Frameworks, kdewin, and Lukáš Tinkl. Changes --- Submitted with commit 31daba692c020943453150a626ed3cf19562676b by Pino Toscano to branch master. Repository: solid Description --- Turn Flex and Bison into

Re: Review Request 127984: Always update the Trader parser from y/l sources

marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit 0cf33ffbf88777a86635948ed03917253342569c by Pino Toscano to branch master. Repository: kservice Description --- Add Flex and Bison as required build dependencies, and use them to always

Re: Review Request 127106: applications.menu: remove references to unused categories

--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127106/#review97247 --- Ping - Pino Toscano On March 28, 2016, 1:41 p.m., Pino

Re: Review Request 127106: applications.menu: remove references to unused categories

marked as submitted. Review request for KDE Frameworks and David Faure. Changes --- Submitted with commit c1ca6fe180d171e365024e1f51f003095ae962fc by Pino Toscano to branch master. Repository: kservice Description --- Stop handling few categories long deprecated (replaced by

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 not,

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, LeG

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 https://phabricator

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 > 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 clea

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 #

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 neat

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 h

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 https://phabricator.kde.org/D2853

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, LeG

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 https:/

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, kde-frameworks-

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 Cc:

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, sit

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 aware;

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 mass-

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 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. In D29299#660466 , @kossebau wrote: > In D29299#660465 , @pino wrote: > > > Also, your patch basically includes D29136 in the case of no DES

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 so

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#660614 , @kossebau wrote: > In D29299#660519 , @pino wrote: > > > In D29299#660505 , @kossebau wrote: > > > > > Because

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 icon

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, nikol

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 replace

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 executin

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 Pla

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 https://

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 ke

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, aprce

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 Applicat

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#6742

D27633: Port to KPluginLoader

pino added a comment. In D27633#619369 , @aacid wrote: > In D27633#619365 , @aacid wrote: > > > I think this broke https://build.kde.org/job/Applications/job/ktp-common-internals/job/kf5-qt5%20SUSEQt

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

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#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 https://

D13549: Switch KIO::convertSize() to KFormat::formatByteSize()

pino created this revision. pino added a reviewer: dfaure. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. pino requested review of this revision. REVISION SUMMARY Make KIO::convertSize() use KFormat::formatByteSize(), instead

D13585: Use LIB_SUFFIX for lmdb libdir rather than hardcode lib

pino added a comment. Or just drop the two `HINTS` (for `find_path` and `find_library`) altogether, as cmake already looks in standard include/library paths. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D13585 To: asturmlechner, #baloo Cc: pino, kde-frameworks-devel

D13585: Use LIB_SUFFIX for lmdb libdir rather than hardcode lib

pino added a comment. In D13585#279400 , @asturmlechner wrote: > The problem with that is cmake picking the wrong dir (first in order is `lib`, while it should be e.g. `lib64`). Sure, and that's why I suggested to drop the usage of `LMDB

D13549: Switch KIO::convertSize() to KFormat::formatByteSize()

pino added a comment. Polite ping. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13549 To: pino, dfaure Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13549: Switch KIO::convertSize() to KFormat::formatByteSize()

pino added inline comments. INLINE COMMENTS > aacid wrote in global.cpp:44 > Am i reading the code wrong or are we doing the readEntry twice? (i know the > old code did the same) > > Do you understand why? > Am i reading the code wrong or are we doing the readEntry twice? (i know the > old co

D13549: Switch KIO::convertSize() to KFormat::formatByteSize()

pino updated this revision to Diff 3. pino marked an inline comment as done. pino added a comment. Fix precision passed to formatByteSize(). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13549?vs=36179&id=3 BRANCH kformat (branched from master) REV

D13549: Switch KIO::convertSize() to KFormat::formatByteSize()

pino marked an inline comment as done. pino added inline comments. INLINE COMMENTS > aacid wrote in global.cpp:58 > Should this -1 be 1? Looking at the kformat code it'll use that unless unit > is 0 so the old code would be using 1 and the new one -1 Indeed, 1 is the right value, and `KFormat`

D13940: Add syntax highlighting support for Stan

pino added a comment. It looks like your forgot to include the actual `highlight.stan` file for the test suite, can you please amend this commit to include it? REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D13940 To: jeffreyarnold, #framework_syntax_hig

D13549: Switch KIO::convertSize() to KFormat::formatByteSize()

pino closed this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13549 To: pino, dfaure, aacid Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D14111: Install MIME type definition for text/x-rst ourselves for now

pino added a comment. NACK. Make sure that the request for shared-mime-info (https://bugs.freedesktop.org/show_bug.cgi?id=107227) is accepted, and that is enough. There is enough stuff in `kde5.xml` already, even shipped in shared-mime-info, so adding more is not a good idea. REPOSITORY

D14111: Install MIME type definition for text/x-rst ourselves for now

pino requested changes to this revision. pino added a comment. This revision now requires changes to proceed. In D14111#291934 , @kossebau wrote: > In D14111#291933 , @pino wrote: > > > NACK. > > M

D14134: KFormat: fix typo in SI prefix name enum

pino requested changes to this revision. pino added a comment. This revision now requires changes to proceed. It must be fixed in `src/lib/util/kformatprivate.cpp` too. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D14134 To: bruns, #frameworks, astippich, pino

D14135: KFormat: fix typo in SI prefix name enum

pino added a comment. Duplicate of D14134: KFormat: fix typo in SI prefix name enum ? If so, please abandon this. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D14135 To: bruns, #frameworks, astippich Cc: pino, kde-frameworks

D14135: KFormat: fix typo in SI prefix name enum

pino accepted this revision. This revision is now accepted and ready to land. REPOSITORY R244 KCoreAddons BRANCH prefix_typo REVISION DETAIL https://phabricator.kde.org/D14135 To: bruns, #frameworks, astippich, pino Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

pino added a comment. TBH creating a widget without adding it to a layout does not make much sense to me -- in the past I saw those situations as "floating" widgets usually anchored at the top-left corner of the parent widget. The best way here is to just not create `m_iconButton` at all, l

D14360: Remove custom icon selection for trash

pino added a comment. In D14360#298208 , @shubham wrote: > pino: I don't why m_iconButton isn't needed(because it is being used by other entries also ), am I right? I don't understandwhat you mean, can you please rephrase it? The poi

D14360: Remove custom icon selection for trash

pino requested changes to this revision. pino added a comment. This revision now requires changes to proceed. It is a good start, but not enough: - `m_iconButton` must be set to null in the constructor (ideally at the beginning, in the initializer list), otherwise it will be uninitialized

D14360: Remove custom icon selection for trash

pino requested changes to this revision. pino added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kfileplaceeditdialog.cpp:66 > +// Get icon except for trash > +if (url.scheme() != QLatin1String("trash")) { > +icon = dialog->icon();

D14360: Remove custom icon selection for trash

pino added a comment. In D14360#299439 , @shubham wrote: > pino, I understood your idea of having a helper class function, but the condition for editing the icon will remain the same ( based on its scheme), what can be the other condition?

D14360: Remove custom icon selection for trash

pino added a comment. In D14360#299441 , @shubham wrote: > indirectly you mean to transfer all the logic to helper function No, I suggested something like: bool KFilePlaceEditDialog::canEditIcon() const { return m_iconButt

D14360: Remove custom icon selection for trash

pino added a comment. In D14360#299443 , @shubham wrote: > pino: suppose this function is created(canEditIcon()),then this function must also be called when we are creating the m_iconButton at line 123 so as to generalize it's creation ( as you

D14360: Remove custom icon selection for trash

pino added a comment. In D14360#301270 , @cfeck wrote: > I still would like to understand why it needs to be public API, instead of just an `@internal` method. This is not a public API, since `KFilePlaceEditDialog` is a private class whi

D14360: Remove custom icon selection for trash

pino requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D14360 To: shubham, ngraham, broulik, #dolphin, #frameworks, pino Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

pino added a comment. In D14360#301497 , @cfeck wrote: > If it's not public, it doesn't need a `@since` tag. No idea, it was not me to suggest that. > Why isn't the header called *_p.h ...? No idea either. REPOSITORY R241 KIO

D14504: Save few string allocations

pino added a comment. TBH I had the idea of making `splitLocale()` public, since it was a public API in `KLocale`. I still see a couple of users of it, and in the past I remember people working around the lack of `splitLocale()` by doing the same job on their own, when porting away from kdel

D14449: Modify device usage information

pino added inline comments. INLINE COMMENTS > kpropertiesdialog.cpp:1189 > +grid->addWidget(l, curRow, 2); > +l->setText(i18nc("Device usage information","%1 used , %2 free", > KIO::convertSize(size - free), KIO::convertSize(free))); > + Also, style for i18n strings: there must

D14757: Warn user before copy operation if available space is not enough

pino requested changes to this revision. pino added a comment. This revision now requires changes to proceed. Also, considering this is in a job in `KIOCore` (i.e. non-gui library), I suspect that using a message box directly is the wrong way to do it. Most probably you need to use the UI del

D14236: Add some improvements to kate-syntax-highlighter for use in scripting

pino added inline comments. INLINE COMMENTS > htmlhighlighter.h:42-43 > void highlightFile(const QString &fileName); > +void highlightFile(const QString &fileName, const QString &title); > +void highlightFile(QFile &file, const QString &title); > Instead of both the variants, what

D14757: Warn user before copy operation if available space is not enough

pino requested changes to this revision. pino added a comment. This revision now requires changes to proceed. Hint: please make sure you build **and** test the next versions of your patches, otherwise it is just a waste of everybody's time. INLINE COMMENTS > copyjob.cpp:891 > +if (m_

D14859: Don't try to call transaction documentUrl with a 0 id

pino requested changes to this revision. pino added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > main.cpp:140 > fid = Baloo::devIdAndInodeToId(devId, inode); > -url = QFile::decodeName(tr.documentUrl(fid)); > +if (fid) { >

D17991: Refactor the way device backends are built and registered

pino added a comment. ping? REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D17991 To: pino Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18770: [KStatusNotifierItem] use fallback sizes when none is available

pino created this revision. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. pino requested review of this revision. REVISION SUMMARY When converting a QIcon to a vector of images to D-Bus, the list of available sizes is used to extract the pixmaps of the ic

D18883: Add PDF thumbnailer

pino added inline comments. INLINE COMMENTS > pdfcreator.cpp:45-46 > +{ > +Q_UNUSED(width); > +Q_UNUSED(height); > + please honor the requested width and height > pdfcreator.cpp:49 > +QScopedPointer document; > +document.reset(Document::load(QFile::encodeName(path))); > + Docum

D18883: Add PDF thumbnailer

pino added inline comments. INLINE COMMENTS > pdfcreator.cpp:23 > + > +#include > +#include the QFile include is no more needed now > broulik wrote in pdfcreator.cpp:45-46 > I can't. The `renderToImage` can only be told a resolution or part of the > page to render, to render downscaled into

D18882: [Image Thumbnailer] Support eps files

pino added a comment. OTOH this will not work if kimageformats is not installed, and the thumbnailer gives no hint about that. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D18882 To: broulik, jtamate, dfaure, ngraham Cc: pino, ngraham, kde-frameworks-devel, kfm

D18882: [Image Thumbnailer] Support eps files

pino added a comment. Then just ship a desktop file for that format in kimageformats: - if kio-extras is not installed, that desktop file will be unused (although just a couple of kilobytes on disk) - if ko-extras is installed, it will register PS as thumbnail format using imagethumbna

  1   2   3   >