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

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

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

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

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

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

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,

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

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

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. 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. In D29805#674218 , @meven wrote: > In D29805#674206 , @pino wrote: > > > In D29805#674205 , @meven wrote: > > > > > In

D27633: Port to KPluginLoader

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

[Differential] [Commented On] D3830: Add a new FindGperf module

pino added a comment. In https://phabricator.kde.org/D3830#72502, @rjvbb wrote: > You're still determining the location from the path with this new revision, right? For the input file? Yes. REPOSITORY R240 Extra CMake Modules REVISION DETAIL

[Differential] [Updated, 125 lines] D3830: Add a new FindGperf module

pino updated this revision to Diff 9502. pino added a comment. Typo fix in previous revision. REPOSITORY R240 Extra CMake Modules CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D3830?vs=9499=9502 BRANCH pino-gperf REVISION DETAIL https://phabricator.kde.org/D3830 AFFECTED

[Differential] [Updated, 125 lines] D3830: Add a new FindGperf module

pino updated this revision to Diff 9499. pino added a comment. Restricted Application added projects: Frameworks, Buildsystem. Use a full path for the output location. REPOSITORY R240 Extra CMake Modules CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D3830?vs=9408=9499 BRANCH

[Differential] [Commented On] D3830: Add a new FindGperf module

pino added a comment. In https://phabricator.kde.org/D3830#72696, @rjvbb wrote: > In https://phabricator.kde.org/D3830#72508, @pino wrote: > > > For the input file? Yes. > > > I did mean the gperf executable...! I don't get what you mean, sorry :/ The full path of gperf

[Differential] [Request, 862 lines] D3883: Generate gperf output at build time

pino created this revision. pino added a reviewer: Frameworks. Restricted Application added a project: Frameworks. REVISION SUMMARY Look for gperf, and use it to generate the hash-based lookup for the entities; this replaces the static generated file in the sources, adding a build time only

[Differential] [Commented On] D3830: Add a new FindGperf module

pino added a comment. Cool -- thanks guys for the feedback on FreeBSD/Mac/Windows! REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D3830 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: pino, #frameworks, #buildsystem,

[Differential] [Request, 126 lines] D3830: Add a new FindGperf module

pino created this revision. pino added a reviewer: Frameworks. REVISION SUMMARY Add a simple module to look for GNU gperf at build time, providing an helper macro for adding generations to a list of sources. gperf will be used to generate the C/C++ sources at build time, instead of

[Differential] [Commented On] D3830: Add a new FindGperf module

pino added a comment. Windows and Mac people: at least from a quick glance, GNU gperf should be already available on Windows and Mac; can you please confirm the new (build time only) dependency could be acceptable? REPOSITORY R240 Extra CMake Modules REVISION DETAIL

[Differential] [Updated] D3830: Add a new FindGperf module

pino added reviewers: Windows, kde-mac. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D3830 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: pino, #frameworks, #kde_buildsystem, #windows, kde-mac

<    1   2   3