D29050: WIP KRunner fix prepare/teardown signals

[Christoph Feck]

cfeck added a project: Plasma.
cfeck added a comment.


  If this is no longer Work-In-Progress, please remove "WIP" from the title.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D29050

To: alex, meven, ngraham, broulik
Cc: cfeck, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, michaelh, ZrenBot, ngraham, bruns, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D29711: Create kcmshell.openSystemSettings() and kcmshell.openInfoCenter() functions

[Christoph Feck]

cfeck added a comment.


  Sorry if I don't understand the scope, but does this mean I am forced to 
install systemsettings to be able to use KCMs?

REPOSITORY
  R296 KDeclarative

REVISION DETAIL
  https://phabricator.kde.org/D29711

To: ngraham, #plasma, mart
Cc: cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kcolorcombo.cpp:89
> +int unused, v;
> +innercolor.getHsv(, , );
> +textColor = v > 128 ? Qt::black : Qt::white;

Please don't use "value" component to calculate the brightness of a color. 
#81 is much darker than #808080. To decide, use qGray(). The threshold also 
needs to be higher than 128; I use 170, but this mostly depends on correctness 
of monitor gamma settings.

See  
https://stackoverflow.com/questions/3942878/how-to-decide-font-color-in-white-or-black-depending-on-background-color

> kcolorcombo.cpp:277
> +list.reserve(d->colorList.size());
> +for (auto iColor : d->colorList) {
> +list.append({iColor.second});

Variables declared in 'for' are local, so just use 'color'.

> kcolorcombo.h:61
>  
> +/** Struct used in named colors list */
> +using ColorList = QList>;

The comment still says "struct". Maybe clarify that this list is actually used 
as a map.

(I guess since mapping would happen in both directions, using a QMap isn't 
useful?)

> kcolorcombo.h:91
> + * standard list.
> + * @param colors map os named colors. If empty, the selction list
> + *  reverts to the standard list.

typos: of; selection

> kcolorcombo.h:97
> +/**
> + * Inserts a named color at a specific position in the standard list
> + * @param index position in the list

Sentence misses a full stop.

> kcolorcombo.h:110
> +/**
> + * Return the list of named colors available for selecion.
> + * If no named color, return namedColor is empty.

typo: selection

> kcolorcombo.h:111
> + * Return the list of named colors available for selecion.
> + * If no named color, return namedColor is empty.
> + * @return list of named colors

This sentence is confusing. I guess you wanted to write "if there are no named 
colors, the returned list is empty." (to clarify that it won't return the 
standard list).

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D29502

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: broulik, cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Christoph Feck]

cfeck added a comment.


  Does the delegate ensure the text is rendered in a color visible over the 
colored background?

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D29502

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Christoph Feck]

cfeck added a comment.


  There is QPalette::Button, but I don't see any hover/focus colors in QPalette.
  
  If we want more roles, we seriously need to contribute them upstream. Qt 6 is 
a chance to avoid diverging more.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D25814

To: ndavis, #frameworks, #vdg
Cc: ahiemstra, broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, 
cfeck, hpereiradacosta, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D25267: Improve XCF support

[Christoph Feck]

cfeck added a comment.


  Thanks for your work, Martin! Could you please add a note (or resolve) bug 
360821?

REPOSITORY
  R287 KImageFormats

REVISION DETAIL
  https://phabricator.kde.org/D25267

To: sandsmark, aacid, cfeck, apol, vkrause
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[Christoph Feck]

cfeck added a comment.


  Are more changes planned?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26407

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: cfeck, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

D27455: FileWidgets: Ignore Return events from KDirOperator

[Christoph Feck]

cfeck edited the summary of this revision.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27455

To: meven, dfaure, ngraham, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27802: smb: fix ipv6 support

[Christoph Feck]

cfeck added a comment.


  Well, cannot delay the release any longer.

REPOSITORY
  R320 KIO Extras

BRANCH
  smb-smburl-static-autotest-ipv6

REVISION DETAIL
  https://phabricator.kde.org/D27802

To: sitter, ngraham
Cc: cfeck, thiago, kde-frameworks-devel, kfm-devel, pberestov, iasensio, 
fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D27802: smb: fix ipv6 support

[Christoph Feck]

cfeck added a comment.


  Should this be in 19.12 branch? I am doing a respin of kio-extras anyway.

REPOSITORY
  R320 KIO Extras

BRANCH
  smb-smburl-static-autotest-ipv6

REVISION DETAIL
  https://phabricator.kde.org/D27802

To: sitter, ngraham
Cc: cfeck, thiago, kde-frameworks-devel, kfm-devel, pberestov, iasensio, 
fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

kreversi 19.12 fails to build at IconSize call

[Christoph Feck]

Hi,

kreversi release/19.12 branch fails to build with this error on CI:


pageItem->setIcon(QIcon::fromTheme(icon).pixmap(IconSize(KIconLoader::Toolbar)));
error: IconSize was not declared in this scope

See 
https://build.kde.org/job/Applications/view/Everything%20-%20stable-kf5-qt5/job/kreversi/


Additionally, kwordquiz fails to build on Windows CI,
see 
https://build.kde.org/job/Applications/view/Everything%20-%20stable-kf5-qt5/job/kwordquiz/


I would be glad if someone could investigate these issues.

Thanks,
Christoph

--
Christoph Feck
KDE Release Team

D27035: [KMessageWidget] Draw it with QPainter instead of using stylesheet

[Christoph Feck]

cfeck accepted this revision.
cfeck added a comment.


  Merci :)

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  no-css (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27035

To: davidre, #frameworks, ngraham, cfeck
Cc: cfeck, dhaumann, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D27035: [KMessageWidget] Draw it with QPainter instead of using stylesheet

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kmessagewidget.cpp:155
>  };
> -
> +// Add bordersize to the margin so it starts from the inner border and 
> doesn't look to cramped
> +q->layout()->setContentsMargins(q->layout()->contentsMargins() + 
> borderSize);

too

> kmessagewidget.cpp:193
> +textLabel->setPalette(palette);
> +// update the Icon in case its recolorable
> +q->setIcon(icon);

typo: it is

> kmessagewidget.cpp:325
> +constexpr float radius = 4 * 0.6;
> +const QRect innerRect = rect().marginsRemoved(QMargins() + borderSize/2);
> +const QColor color = palette().color(QPalette::Window);

missing spaces around `/`

> kmessagewidget.cpp:397
>  
> -if (isVisible() && (d->timeLine->state() == QTimeLine::NotRunning) && 
> (height() == d->bestContentHeight()) && (d->content->pos().y() == 0)) {
> +if (isVisible() && (d->timeLine->state() == QTimeLine::NotRunning) && 
> (height() == d->bestContentHeight())){
>  emit showAnimationFinished();

missing space before `{`

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  no-css (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27035

To: davidre, #frameworks, ngraham
Cc: cfeck, dhaumann, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

Re: 2 kirigami fixes for a point release

[Christoph Feck]

On 02/13/20 08:42, Ben Cooksley wrote:

Part of the issue here is that Plasma has been known to add API to
Frameworks and then immediately, without any delay, start using it
(pretty much always breaking CI in the process)

This means that other changes are likely being pushed into Frameworks
by Plasma with very little delay as well.

Consequently this means stuff is landing in Framework repositories up
to the very moment it is released - a release that the next version of
Plasma (LTS) then depends on.

A better way of approaching this would be to freeze the Frameworks
version you are going to require API wise at an earlier point in the
Plasma development cycle. This would allow for a full Frameworks
release cycle to pass where bugs encountered during the lead up to the
Plasma release can be fixed.


You can find bugs in new code best if you are actually using it. I doubt 
that delaying using new API would help much.

D27195: Change "Redisplay" to "Refresh"

[Christoph Feck]

cfeck added a comment.


  Refresh is fine, Reload could be irritating for applications that don't load 
anything, but still know the user could be faced with a view state that needs 
redrawing, e.g. when cleaning corruption caused by non-integer scale factors.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27195

To: ngraham, #vdg, The-Feren-OS-Dev, ndavis
Cc: cfeck, kde-frameworks-devel, The-Feren-OS-Dev, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D27087: Add HEIF thumbnailer

[Christoph Feck]

cfeck added a comment.


  Where can we see the status of the HEIF Qt loader? If it correctly implements 
scaled image loading, then a separate thumbnailer indeed makes not much sense. 
If, however, it always loads the full image, then scales it down, we got a case 
for a separate thumbnailer such as this one.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D27087

To: zzag, #plasma
Cc: cfeck, broulik, ngraham, kde-frameworks-devel, kfm-devel, pberestov, 
iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, andrebarros, bruns, 
emmanuelp, mikesomov

Re: Recommendation: drop ProvidersUrl entry to rely on default value

[Christoph Feck]

On 01/30/20 13:53, Friedrich W. H. Kossebau wrote:

as found out by discussion on irc, a good solution for everyone relying on the
default GHNS storage as provided by KDE is to just not hard-code any value for
ProvidersUrl, but leave it out and let KNewStuff default to what is built into
the KNewStuff library as current value.


Does it work with all KNewStuff 5.x versions? Otherwise, the required
KF5 version would need to be bumped where this change was made.

--
Christoph Feck

D27035: [KMessageWidget] Draw it with QPainter instead of using stylesheet

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kmessagewidget.cpp:39
> +
> +constexpr int borderSize = 2;
> +

Does it need to be `static` to avoid an external symbol?

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  no-css (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27035

To: davidre, #frameworks, ngraham
Cc: cfeck, dhaumann, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D25814: [KColorScheme] Add SeparatorColor

[Christoph Feck]

cfeck added a comment.


  > Why don't focus and hover colors belong?
  
  Because an application cannot know if and how a style does indicate focus or 
hovering.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D25814

To: ndavis, #frameworks, #vdg
Cc: broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, cfeck, 
hpereiradacosta, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26648: Improved quality of JPEG thumbnails

[Christoph Feck]

cfeck accepted this revision.
cfeck added a comment.
This revision is now accepted and ready to land.


  Thanks for the detailed investigation, Stefan!

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D26648

To: chroniceel, broulik, #frameworks, #vdg, cfeck
Cc: meven, volkov, cfeck, bruns, ngraham, kde-frameworks-devel, kfm-devel, 
pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp, mikesomov

D26648: Improved quality of JPEG thumbnails

[Christoph Feck]

cfeck added a comment.


  @meven, this isn't about save quality, but load quality.

REVISION DETAIL
  https://phabricator.kde.org/D26648

To: chroniceel, broulik, #frameworks, #vdg
Cc: meven, volkov, cfeck, bruns, ngraham, kde-frameworks-devel, kfm-devel, 
pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp, mikesomov

D26648: Improved quality of JPEG thumbnails

[Christoph Feck]

cfeck added a comment.


  Did you time a performance difference? I think the idea was to get the 
downscaled result as fast as possible, by using integer IDCT instead of float 
IDCT, and by using only the DC coefficient instead of performing the IDCT at 
all for scale factors <= 1/8.

REVISION DETAIL
  https://phabricator.kde.org/D26648

To: chroniceel, broulik, #frameworks, #vdg
Cc: cfeck, bruns, ngraham, kde-frameworks-devel, kfm-devel, pberestov, 
iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp, mikesomov

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Christoph Feck]

cfeck added a comment.


  Nice :)

INLINE COMMENTS

> krecentfilesmenu.cpp:34
> +
> +KRecentFilesMenu::KRecentFilesMenu(const QString& title, QWidget* parent)
> +: QMenu(title, parent)

Here you switch from `Type *var` to `Type* var`

> krecentfilesmenu.cpp:41
> +
> +KRecentFilesMenu::KRecentFilesMenu(QWidget* parent)
> +: QMenu(tr("Recent Files"), parent)

same

> krecentfilesmenu.cpp:81
> +
> +void KRecentFilesMenu::addUrl(const QUrl url, const QString& name)
> +{

same

> krecentfilesmenu.cpp:103
> +
> +void KRecentFilesMenu::removeUrl(const QUrl& url)
> +{

same

> krecentfilesmenu.cpp:215
> +{
> +d->m_maximumItems = maximumItems;
> +}

Should we truncate the current list if the new max items is smaller?

> krecentfilesmenu.h:67
> + */
> +void addUrl(const QUrl url, const QString  = QString());
> +

Missing reference

> krecentfilesmenu.h:79
> + *
> + * When the manimum url count is reached and a new URL is added the
> + * oldest will be replaced.

manimum url → maximum URL

> krecentfilesmenu.h:104
> +void rebuildMenu();
> +std::list>::const_iterator findUrl(const QUrl url);
> +

Weren't there ABI issues with std::list?

Also, missing reference on url.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D26448

To: nicolasfella, #frameworks, dfaure
Cc: cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Christoph Feck]

cfeck added a comment.


  I also agree with the concerns rised by Hugo. An application will use various 
frame primitives, and the style decides how to draw them. It doesn't belong in 
the API, though (but neither do Focus and Hover colors).

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D25814

To: ndavis, #frameworks, #vdg
Cc: filipf, cfeck, hpereiradacosta, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25697: Port away from KIconLoader::SizeSmallMedium

[Christoph Feck]

cfeck added a comment.


  Traditionally, 22 was used for toolbar icons. You could replace 22 with 
QStyle::PM_ToolBarIconSize, but it really depends on where you want the icons 
to appear. Generally, QStyle::PM_SmallIconSize is for icons that are placed 
next to a single line of text, unless there is a more specifc PixelMetric enum, 
e.g. QStyle::PM_ButtonIconSize.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D25697

To: nicolasfella, #frameworks
Cc: cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25561: Remove unused signal we can use directly "signal(const QUrl&)

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kurllabel.h:281
>   * Emitted when the mouse has passed over the label.
> + * @deprecated Since 5.65, use leftUrl(const QString );
>   */

Wrong comment?

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D25561

To: mlaurent, dfaure, kossebau
Cc: cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D7010: KSqueezedTextLabel: call updateGeometry() when text changes

[Christoph Feck]

cfeck resigned from this revision.
cfeck added a comment.
This revision is now accepted and ready to land.


  Yes, I would prefer the check condition. Feel free to commandeer; it looks 
like the original author didn't find time to further investigate.

REVISION DETAIL
  https://phabricator.kde.org/D7010

To: brauch, dfaure
Cc: rkflx, dfaure, dhaumann, aacid, #frameworks, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25444: Fix assistant dialog margins

[Christoph Feck]

cfeck added a comment.


  Since KAssistantDialog is a KPageDialog, what happens to other users of that 
class? I had hoped the fix is in KPageDialog, and it would only omit the 
margins when the caller requested it (e.g. KCMs etc.).

INLINE COMMENTS

> kassistantdialog.cpp:103
> +q->pageWidget()->layout()->setContentsMargins(
> +style->pixelMetric(QStyle::PM_LayoutLeftMargin),
> +style->pixelMetric(QStyle::PM_LayoutTopMargin),

Please also pass styleOptions and widget. Some styles might compute margins 
from font sizes, and without any context, they can only assume defaults.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D25444

To: ngraham, #frameworks, cfeck, mart
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24993: Add integrated inline spelling menu to KTextEdit

[Christoph Feck]

cfeck added a comment.


  Oh, I didn't read the code, only replied to the comment. If this is new code, 
then changing the visibility is no ABI problem.

REPOSITORY
  R310 KTextWidgets

REVISION DETAIL
  https://phabricator.kde.org/D24993

To: pajamapants3000, #vdg, #frameworks, cullmann, cfeck
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D24993: Add integrated inline spelling menu to KTextEdit

[Christoph Feck]

cfeck added a comment.


  I am not sure if changing the visibility of members is ABI compatible. This 
might need to wait for KF6.

REPOSITORY
  R310 KTextWidgets

REVISION DETAIL
  https://phabricator.kde.org/D24993

To: pajamapants3000, #vdg, #frameworks, cullmann, cfeck
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

Re: New Framework Review: KDAV

[Christoph Feck]

Hi Volker,

On 11/09/19 12:33, Volker Krause wrote:

during Akademy there was a request to promote KDAV from KDE PIM to Frameworks
for use by Plasma Mobile. KDAV is a framework that implements the CalDav/
CardDav/GroupDav protocol on top of KIO's WebDav support. It would be
classified as a functional tier 3 framework.


Could you clarify if the review is about 
https://api.kde.org/kdepim/kdav/html/index.html or 
https://api.kde.org/kdepim/kdav2/html/index.html ?


Christoph Feck

D24672: GIT_SILENT Run uncrustify-kf5 on the whole tree

[Christoph Feck]

cfeck added a reviewer: mkoller.

REPOSITORY
  R374 KolourPaint

REVISION DETAIL
  https://phabricator.kde.org/D24672

To: ahmadsamir, kde-frameworks-devel, mkoller
Cc: ognarb, kde-frameworks-devel

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Christoph Feck]

cfeck added a comment.


  Does it happen with every code that uses QPropertyAnimation, or just with 
this KBusyIndicator?

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck, apol
Cc: rjvbb, ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
GB_2, michaelh, bruns

D24367: Some sanity verification

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> pcx.cpp:312
> +qWarning() << "Failed to get scanline for" << y << "might be out 
> of bounds";
> +}
>  for (int x = 0; x < header.width(); ++x) {

If `p` indeed could be zero, the next statements need to be in an `else` block 
(or a add a `return`).

REPOSITORY
  R287 KImageFormats

REVISION DETAIL
  https://phabricator.kde.org/D24367

To: sandsmark, aacid, cfeck
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24252: Make OK button configurable in KMessageBox::sorry/detailedSorry

[Christoph Feck]

cfeck added a comment.


  I think it needs to be named 'buttonOk' instead of 'buttonOK'.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D24252

To: dfaure, cfeck, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24254: [KCollapsibleGroupBox] Fix QTimeLine::start warning at runtime

[Christoph Feck]

cfeck accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  2019_09_fix_qtimeline_warning (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D24254

To: dfaure, cfeck, ngraham, elvisangelaccio
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Christoph Feck]

cfeck added a comment.


  I would suggest to commit it either sooner, or after 5.63 is tagged. If you 
commit on 3rd, there are only two days to test and decide how to improve or 
revert before tars are made.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D21721

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: cfeck, davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D24113: xcf: Properly read image resolution

[Christoph Feck]

cfeck added a comment.


  That's bug 362484, please close. Merci!

REPOSITORY
  R287 KImageFormats

REVISION DETAIL
  https://phabricator.kde.org/D24113

To: aacid, cfeck, apol, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23811: [KImageFormats] Port HDR (Radiance RGBE) image loader to Qt5

[Christoph Feck]

cfeck added a comment.


  I somehow knew autotest would fail on FreeBSD ... I cannot investigate why it 
fails, maybe too old MIME database?

REPOSITORY
  R287 KImageFormats

REVISION DETAIL
  https://phabricator.kde.org/D23811

To: cfeck, #frameworks, aacid
Cc: aacid, kde-frameworks-devel, LeGast00n, tommo, GB_2, clintmoyer, tdarboux, 
huoni, michaelh, muhlenpfordt, rkflx, ngraham, bruns

D23811: [KImageFormats] Port HDR (Radiance RGBE) image loader to Qt5

[Christoph Feck]

This revision was automatically updated to reflect the committed changes.
Closed by commit R287:68bb1a0ee7f9: Port HDR (Radiance RGBE) image loader to 
Qt5 (authored by cfeck).

REPOSITORY
  R287 KImageFormats

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23811?vs=65784=66041

REVISION DETAIL
  https://phabricator.kde.org/D23811

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/read/hdr/rgb.hdr
  autotests/read/hdr/rgb.png
  src/imageformats/CMakeLists.txt
  src/imageformats/hdr.cpp
  src/imageformats/hdr.json
  src/imageformats/hdr_p.h

To: cfeck, #frameworks, aacid
Cc: aacid, kde-frameworks-devel, LeGast00n, tommo, GB_2, clintmoyer, tdarboux, 
huoni, michaelh, muhlenpfordt, rkflx, ngraham, bruns

D23904: Add QIcon setters for the password dialogs

[Christoph Feck]

cfeck added a comment.


  A getter would be nice. I am also fine with deprecating the pixmap calls. You 
can create an icon from a pixmap using QIcon.
  
  (Why is there no setIcon() in QLabel?)

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D23904

To: vkrause, dfaure
Cc: cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23811: [KImageFormats] Port HDR (Radiance RGBE) image loader to Qt5

[Christoph Feck]

cfeck updated this revision to Diff 65784.
cfeck added a comment.


  Add hdr to autotests

REPOSITORY
  R287 KImageFormats

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23811?vs=65713=65784

REVISION DETAIL
  https://phabricator.kde.org/D23811

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/read/hdr/rgb.hdr
  autotests/read/hdr/rgb.png
  src/imageformats/CMakeLists.txt
  src/imageformats/hdr.cpp
  src/imageformats/hdr.json
  src/imageformats/hdr_p.h

To: cfeck, #frameworks
Cc: aacid, kde-frameworks-devel, LeGast00n, tommo, GB_2, clintmoyer, tdarboux, 
huoni, michaelh, muhlenpfordt, rkflx, ngraham, bruns

D23811: [KImageFormats] Port HDR (Radiance RGBE) image loader to Qt5

[Christoph Feck]

cfeck created this revision.
cfeck added a reviewer: Frameworks.
cfeck added a project: Gwenview.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
cfeck requested review of this revision.

TEST PLAN
  Tested with HDR images from hdrihaven.com
  
  - Loading in KolourPaint works
  - Thumbnails in Dolphin work

REPOSITORY
  R287 KImageFormats

REVISION DETAIL
  https://phabricator.kde.org/D23811

AFFECTED FILES
  src/imageformats/CMakeLists.txt
  src/imageformats/hdr.cpp
  src/imageformats/hdr.json
  src/imageformats/hdr_p.h

To: cfeck, #frameworks
Cc: kde-frameworks-devel, LeGast00n, tommo, GB_2, clintmoyer, tdarboux, huoni, 
michaelh, muhlenpfordt, rkflx, ngraham, bruns

D23797: [KWidgetAddons] port to non-deprecated Qt API

[Christoph Feck]

cfeck accepted this revision.
cfeck added a comment.
This revision is now accepted and ready to land.


  Merci!

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23797

To: dfaure, cfeck
Cc: dhaumann, aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D23797: [KWidgetAddons] port to non-deprecated Qt API

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kpopupframe.cpp:24
>  
> -#include 
> +#include 
>  #include 

Please keep this sorted.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D23797

To: dfaure, cfeck
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23597: Bulk port away from foreach

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kacceleratormanagertest.cpp:35
> +const auto menuActions = menu.actions();
> +for (const QAction* action : menuActions) {
>  if (action->isSeparator()) {

Please use KF5 coding style: `Type *var` instead of `Type* var` (also for `&` 
references),

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D23597

To: kossebau, #frameworks, cfeck
Cc: dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22884: [RFC] Don't show title on page by default

[Christoph Feck]

cfeck requested changes to this revision.
cfeck added a comment.
This revision now requires changes to proceed.


  The header title is usually more descriptive (longer) than the icon name, so 
no.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22884

To: ngraham, #vdg, mart, cfeck
Cc: cfeck, mmustac, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, 
ngraham, bruns

D22525: kioclient: Don't convert `:x:y` to `?line=x=y` for URLs starting with remote schemes.

[Christoph Feck]

cfeck added a comment.


  Oh, if the latter syntax also works, then you are right.

REPOSITORY
  R126 KDE CLI Utilities

REVISION DETAIL
  https://phabricator.kde.org/D22525

To: arrowd
Cc: cfeck, plasma-devel, #frameworks, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D13867: [KMessageWidget] Pass widget to standardIcon()

[Christoph Feck]

cfeck accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D13867

To: broulik, #frameworks, cfeck, ngraham
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22525: kioclient: Don't convert `:x:y` to `?line=x=y` for URLs starting with remote schemes.

[Christoph Feck]

cfeck added a comment.


  Could we only apply the `:xx` check on the filename part, not on the server 
part? Someone might expect that `http://path.to/file.txt:99` also works for 
remote files.

REPOSITORY
  R126 KDE CLI Utilities

REVISION DETAIL
  https://phabricator.kde.org/D22525

To: arrowd
Cc: cfeck, plasma-devel, #frameworks, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Christoph Feck]

cfeck accepted this revision.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck, apol
Cc: ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
sbergeron, michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kbusyindicatorwidget.h:52
> + *
> + * @since 5.60.0
> + */

5.61.0

> kbusyindicatorwidgettest.cpp:43
> +
> +auto toggle = new QPushButton(QStringLiteral("Toggle Visibile"), 
> );
> +

typo

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck, apol
Cc: ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
sbergeron, michaelh, bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[Christoph Feck]

cfeck resigned from this revision.
cfeck added a comment.


  You wrote "ported from Device Notifier". I haven't check in detail yet, but 
do other copyright holders need to be mentioned?

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> sitter wrote in kbusyindicatorwidget.h:48
> Mimics the QML API, haven't given it any thought TBH. You can have the 
> spinner visible but paused, I am not sure why exactly you'd want a paused 
> spinner but that's what the property does anyway.
> 
> i.e.
> 
> - you can have a visible but !running spinner which would simply be the 
> spinner at whatever the last rotation update was.
> - a visibile and running spinner = animation
> - a running but !visible spinner = effectively noop
> 
> That said, **I** have no use case for it so we can remove it for the time 
> being if you'd prefer.

Please remove it unless you find a justification. I don't like its name either 
(the widget isn't running).

> kbusyindicatorwidget.h:72
> +class Private;
> +Private *d;
> +};

`*const d`

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck
Cc: broulik, kde-frameworks-devel, apol, LeGast00n, michaelh, ngraham, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kbusyindicatorwidget.h:48
> + */
> +Q_PROPERTY(bool running READ running WRITE setRunning NOTIFY 
> runningChanged)
> +public:

Why is this property needed? If the (parent) widget is no longer busy, this 
spinner needs to be hidden anyway.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck
Cc: broulik, kde-frameworks-devel, apol, LeGast00n, michaelh, ngraham, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Christoph Feck]

cfeck added a comment.


  Ideally you can create it with any size as an overlay to an existing widgets 
(also to block input there), but the spinner itself is only rendered at a 
smaller centered position.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck
Cc: broulik, kde-frameworks-devel, apol, LeGast00n, michaelh, ngraham, bruns

D22083: introduce concept of header and footer for kpageview

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kpageview.cpp:458
> +
> +if (d->pageHeader == header) {
> +return;

This check is duplicated.

> kpageview.cpp:476
> +}
> +
> +}

Please remove this empty line.

> kpageview.h:163
> +/**
> + * Set a widget as the header for this Page view
> + * It will replace the standard page title

Does this transfer ownership of the widget? If yes, is ownership transferred 
back for any previously set widget? Or is it even automatically deleted? It 
needs to be clarified in the docs.

> kpageview_p.h:81
>  
> +QPointer  pageHeader;
> +QPointer  pageFooter;

No space before `<`

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22083

To: mart, #plasma, #frameworks, #vdg
Cc: cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D21894: set the focusPolicy of kpasswordlineedit to the policy of its proxy

[Christoph Feck]

cfeck accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D21894

To: sitter, cfeck
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D18161: [kioslave/file] Add a codec for legacy filenames

[Christoph Feck]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:6738a8b2f71c: [kioslave/file] Add a codec for legacy 
filenames (authored by cfeck).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D18161?vs=49684=58399#toc

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18161?vs=49684=58399

REVISION DETAIL
  https://phabricator.kde.org/D18161

AFFECTED FILES
  src/ioslaves/file/CMakeLists.txt
  src/ioslaves/file/file.cpp
  src/ioslaves/file/legacycodec.cpp
  src/ioslaves/file/legacycodec.h

To: cfeck, #frameworks, #dolphin, dfaure
Cc: frispete, nathanshearer, nerdopolist, ngraham, kde-frameworks-devel, 
michaelh, bruns

D18161: [kioslave/file] Add a codec for legacy filenames

[Christoph Feck]

cfeck retitled this revision from "[WIP/RFC] [kioslave/file] Add a codec for 
legacy filenames" to "[kioslave/file] Add a codec for legacy filenames".
cfeck edited the summary of this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D18161

To: cfeck, #frameworks, #dolphin, dfaure
Cc: frispete, nathanshearer, nerdopolist, ngraham, kde-frameworks-devel, 
michaelh, bruns

D21028: add multiple gestures and a handler class to KWidgetsAddons

[Christoph Feck]

cfeck added a comment.


  Did you try to submit the additional gesture types to Qt? We should only add 
them here if they rejected the idea to add new types in Qt.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D21028

To: steffenh
Cc: cfeck, kde-frameworks-devel, michaelh, ngraham, bruns

D18161: [WIP/RFC] [kioslave/file] Add a codec for legacy filenames

[Christoph Feck]

cfeck added a comment.


  The encode/decode functions were already reviewed for kdelibs4. It's the 
remaining code that needs review.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D18161

To: cfeck, #frameworks, #dolphin, dfaure
Cc: frispete, nathanshearer, nerdopolist, ngraham, kde-frameworks-devel, 
michaelh, bruns

D20181: Add Binary Data units (bits, kilobytes, kibibytes ... yottabytes)

[Christoph Feck]

cfeck added a comment.


  KFormat knows about the prefixes, but doesn't know their name. I would say 
adding translations for "megabytes" etc. to KCoreAddons is out of scope.

REVISION DETAIL
  https://phabricator.kde.org/D20181

To: JJRcop, broulik, #plasma, ngraham
Cc: cfeck, apol, aacid, ngraham, kde-frameworks-devel, michaelh, bruns

D20766: Use appropriate background color for text previews

[Christoph Feck]

cfeck requested changes to this revision.
cfeck added a comment.


  Before we can support both dark and light themes we need to investigate the 
KSyntaxHighlighting issue found by Friedrich.
  
  Later we could request a light or dark theme depending on the lightness of 
the QPalette entry, but I would suggest to use View instead of Window, because 
window backgrounds are gray with some themes, while view backgrounds usually 
have more contrast (either darker or brighter than gray).

REVISION DETAIL
  https://phabricator.kde.org/D20766

To: eshalygin, kossebau, cfeck
Cc: vkrause, cfeck, meven, broulik, kde-frameworks-devel, kfm-devel, alexde, 
feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, 
bruns, emmanuelp, mikesomov

D20489: [KIO] Make it compile without foreach (Step 1)

[Christoph Feck]

cfeck added a comment.


  Do we understand all possible regressions that this can cause? If yes, where 
are they documented so that we can verify? See bug 406426.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D20489

To: mlaurent, dfaure
Cc: cfeck, aacid, cgiboudeaux, kde-frameworks-devel, michaelh, ngraham, bruns

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

[Christoph Feck]

cfeck accepted this revision.
cfeck added a comment.
This revision is now accepted and ready to land.


  KF5 coding style:
  
if (...) {
...
}

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D20506

To: dfaure, cfeck
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20434: KateIconBorder: Use UTF-8 char instead of special pixmap as dyn wrap indicator

[Christoph Feck]

cfeck added a comment.


  The proposed character is in Unicode since version 1.1 (1993).

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D20434

To: loh.tar, cullmann, #ktexteditor
Cc: cfeck, ngraham, kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, 
domson, michaelh, bruns, demsking, cullmann, sars, dhaumann

D19771: Use placeholder instead of label

[Christoph Feck]

cfeck added a comment.


  In D19771#431674 , @ngraham wrote:
  
  > - "Find" is limited to items in the current view only, and usually pertains 
to text.
  
  
  I prefer the term "Filter" for this. If there is no filter, you see 
everything, if there is a filter, you only see the items that match. No 
additional search is performed.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19771

To: ognarb, #ktexteditor, #vdg
Cc: cfeck, loh.tar, ngraham, kwrite-devel, kde-frameworks-devel, gennad, 
domson, michaelh, bruns, demsking, cullmann, sars, dhaumann

D18915: Fix batchrename changing extension to lower case

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> cfeck wrote in batchrenamejob.cpp:165
> Function/method names are usually lowercase. Also, we don't add `get` for 
> getters, only `set` for setters.
> 
> ⇒ `fileExtension()` ?
> 
> Additionally, we pass QString via reference
> 
> ⇒ `QString ` ?

Actually, const reference:

`const QString `

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D18915

To: cfoster, #dolphin, #frameworks, abalaji
Cc: cfeck, bruns, ngraham, elvisangelaccio, chinmoyr, kde-frameworks-devel, 
michaelh

D18915: Fix batchrename changing extension to lower case

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> bruns wrote in batchrenamejob.cpp:165
> `QString extension = GetFileExtension(url.fileName());`
> ...
> `static QString BatchRenameJobPrivate::GetFileExtension(QString filename)`

Function/method names are usually lowercase. Also, we don't add `get` for 
getters, only `set` for setters.

⇒ `fileExtension()` ?

Additionally, we pass QString via reference

⇒ `QString ` ?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D18915

To: cfoster, #dolphin, #frameworks, abalaji
Cc: cfeck, bruns, ngraham, elvisangelaccio, chinmoyr, kde-frameworks-devel, 
michaelh

T10554: KDiff3 to join Applications

[Christoph Feck]

cfeck added a comment.


  Moving KDiff3 to KDE Applications was suggested at 
https://marc.info/?l=kde-core-devel=155055818529475=2
  
  In https://phabricator.kde.org/T10546 I instructed Michael to ask developers 
for a quick review of the (many) changes made for the KF5 port.
  
  Apparently Michael prefers Phabricator task to mailing list postings ...

TASK DETAIL
  https://phabricator.kde.org/T10554

To: cfeck
Cc: cfeck, aacid, kde-frameworks-devel, mreeves

D19432: [text thumbnailer] Use KSyntaxHighlighting for text rendering

[Christoph Feck]

cfeck accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R320 KIO Extras

BRANCH
  addsyntaxhighlighttotextpreview

REVISION DETAIL
  https://phabricator.kde.org/D19432

To: kossebau, broulik, cfeck
Cc: vkrause, cfeck, kde-frameworks-devel, kfm-devel, alexde, feverfew, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D19432: [text thumbnailer] Use KSyntaxHighlighting for text rendering

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> textcreator.cpp:169
> +
> syntaxHighlighter.setDefinition(m_highlightingRepository.definitionForFileName(path));
> +
> syntaxHighlighter.setTheme(m_highlightingRepository.defaultTheme(KSyntaxHighlighting::Repository::LightTheme));
> +

KSyntaxHighlighting::Theme also provides a background color. Can that be used 
instead of the hardcoded 245? I cannot remember reason why I disabled (or never 
enabled) the QPalette code; I suggest to remove it.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D19432

To: kossebau, broulik
Cc: cfeck, kde-frameworks-devel, kfm-devel, alexde, feverfew, michaelh, 
spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

D19170: Fix crash while moving files

[Christoph Feck]

cfeck added a comment.


  That a nested event loop of an error dialog caused the crashes makes sense, 
because they were reported for alien drives (NTFS) or transfers with permission 
problems. What actual error dialogs did you get and are they reproducible?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D19170

To: hallas, #frameworks, elvisangelaccio, dfaure
Cc: cfeck, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D18849: [KPropertiesDialog] Fix group combobox

[Christoph Feck]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:2bba2c0795d7: [KPropertiesDialog] Fix group combobox 
(authored by cfeck).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18849?vs=51180=52017

REVISION DETAIL
  https://phabricator.kde.org/D18849

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp

To: cfeck, #frameworks, #dolphin, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18968: Word-wrap KMessageWidget text

[Christoph Feck]

cfeck added a comment.


  Dolphin could simply call KStringHandler::*squeeze(), maybe only on the 
actual filepath, if squeezing is preferred to wrapping.

REPOSITORY
  R318 Dolphin

BRANCH
  word-wrap-long-kmessagewidget-text (branched from Applications/18.12)

REVISION DETAIL
  https://phabricator.kde.org/D18968

To: ngraham, #dolphin, #frameworks, cfeck
Cc: elvisangelaccio, cfeck, kfm-devel, alexde, feverfew, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, mikesomov

D19056: Tell people they should mostly be using KF5::AuthCore

[Christoph Feck]

cfeck retitled this revision from "Tell people they should mostly be using 
KF5::Auth" to "Tell people they should mostly be using KF5::AuthCore".

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D19056

To: aacid
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18968: Word-drap KMessageWidget text

[Christoph Feck]

cfeck added a comment.


  Well, fix the TODO, and you can remove the comment ;)
  
  I guess QLabel needs new API to set 
https://doc.qt.io/qt-5/qtextoption.html#WrapMode-enum
  
  But those plans should be tracked elsewhere, not in source. IMHO, you or 
maintainer decides, not me.

REPOSITORY
  R318 Dolphin

BRANCH
  word-wrap-long-kmessagewidget-text (branched from Applications/18.12)

REVISION DETAIL
  https://phabricator.kde.org/D18968

To: ngraham, #dolphin, #frameworks, cfeck
Cc: cfeck, kfm-devel, alexde, feverfew, spoorun, navarromorales, firef, 
andrebarros, emmanuelp, mikesomov

D18968: Word-drap KMessageWidget text

[Christoph Feck]

cfeck accepted this revision.
cfeck added a comment.
This revision is now accepted and ready to land.


  I would just remove the comment. The commit references the bug anyway, and a 
bug reference in the code is only helpful if the code is wrong, and someone 
wants to understand why.

REPOSITORY
  R318 Dolphin

BRANCH
  word-wrap-long-kmessagewidget-text (branched from Applications/18.12)

REVISION DETAIL
  https://phabricator.kde.org/D18968

To: ngraham, #dolphin, #frameworks, cfeck
Cc: cfeck, kfm-devel, alexde, feverfew, spoorun, navarromorales, firef, 
andrebarros, emmanuelp, mikesomov

D18731: Replace KIconThemes dependency with equivalent QIcon usage

[Christoph Feck]

cfeck accepted this revision.
cfeck added a comment.


  But KDE also supports

REPOSITORY
  R294 KBookmarks

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D18731

To: vkrause, davidedmundson, cfeck
Cc: broulik, cfeck, kde-frameworks-devel, michaelh, ngraham, bruns

D18849: [KPropertiesDialog] Fix group combobox

[Christoph Feck]

cfeck created this revision.
cfeck added reviewers: Frameworks, Dolphin.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
cfeck requested review of this revision.

REVISION SUMMARY
  `groupList` was never initialized with `KUser::groupNames()`, causing the 
subsequent combobox setup to be skipped.
  
  BUG: 403074

TEST PLAN
  `kpropertiesdialogtest` shows a combobox with `users` and `audio` groups on 
my system. Without this patch, no combobox appears.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D18849

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp

To: cfeck, #frameworks, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18731: Replace KIconThemes dependency with equivalent QIcon usage

[Christoph Feck]

cfeck added a comment.


  Are application-specific icons now accessible with QIcon::fromTheme? Think 
Konqueror favicons in bookmark menu.
  
  Also, according to 
https://api.kde.org/frameworks/kbookmarks/html/kbookmarks-dependencies.html 
KBookmarks indirectly depends on KIconThemes via KTextWidgets anyway.

REPOSITORY
  R294 KBookmarks

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D18731

To: vkrause, davidedmundson
Cc: cfeck, kde-frameworks-devel, michaelh, ngraham, bruns

D18699: [KIO/RenameDialog] Add new apply behaviour

[Christoph Feck]

cfeck added a comment.


  The "Apply to All" button is used for all choices, including "Overwrite", 
"Skip", and "Rename".

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D18699

To: chinmoyr, dfaure, ngraham, #vdg
Cc: cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D18563: Don't create directory tree when a new folder has a '/' in the name

[Christoph Feck]

cfeck added a comment.


  It could ask what to do.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D18563

To: shubham, ngraham, #frameworks, #dolphin, dfaure, elvisangelaccio, pino
Cc: cfeck, acrouthamel, markg, ndavis, dfaure, elvisangelaccio, pino, 
kde-frameworks-devel, michaelh, ngraham, bruns

D18380: KIO: make file dialog columns resizable again (and movable)

[Christoph Feck]

cfeck added a comment.


  As far as I know, using qobject_cast is faster than comparing class names, 
because it only compares metaclass pointers. Additionally, it allows subclasses.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D18380

To: rjvbb, ngraham, #frameworks, #dolphin, apol, dfaure, ahartmetz
Cc: cfeck, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns

D18380: KIO: make file dialog columns resizable again (and movable)

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kdiroperatordetailview.cpp:54
> +}
> +m_isFileWidget = pw ? strcmp(pw->metaObject()->className(), 
> "KFileWidget") == 0 : false;
> +// install the section resize handler

Can we use qobject_cast here?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D18380

To: rjvbb, ngraham, #frameworks, #dolphin, apol, dfaure, ahartmetz
Cc: cfeck, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns

D18161: [WIP/RFC] [kioslave/file] Add a codec for legacy filenames

[Christoph Feck]

cfeck added a comment.


  Thanks for testing, btw.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D18161

To: cfeck, #frameworks, #dolphin, dfaure
Cc: nerdopolist, ngraham, kde-frameworks-devel, michaelh, bruns

D18161: [WIP/RFC] [kioslave/file] Add a codec for legacy filenames

[Christoph Feck]

cfeck added a comment.


  It's possible that Kate doesn't use KIO for local files, but falls back to 
QFile. It _would_ be possible somehow (e.g. via the QPA plugins) to inject the 
hack into all Qt applications, but I suggest to improve Dolphin in a way that 
it shows an error message or even a rename dialog when trying to open or 
execute such a file.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D18161

To: cfeck, #frameworks, #dolphin, dfaure
Cc: nerdopolist, ngraham, kde-frameworks-devel, michaelh, bruns

D17816: Support for xattrs on kio copy/move

[Christoph Feck]

cfeck added a comment.


  I am still not fond of having a local event loop in KIO. The whole point of 
KIO is that it should work asynchronously.
  
  If the tests fail, could they be adapted? Or is the reason why the tests fail 
unfixable?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: cochise, dfaure
Cc: cfeck, bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, 
ngraham, atha.kane, spoorun, nicolasfella, kde-frameworks-devel, michaelh

D18161: [WIP/RFC] [kioslave/file] Add a codec for legacy filenames

[Christoph Feck]

cfeck updated this revision to Diff 49684.
cfeck added a comment.


  Fix worst case estimate. We need up to two UTF-16 code units for each byte.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18161?vs=49176=49684

REVISION DETAIL
  https://phabricator.kde.org/D18161

AFFECTED FILES
  src/ioslaves/file/CMakeLists.txt
  src/ioslaves/file/file.cpp
  src/ioslaves/file/legacycodec.cpp
  src/ioslaves/file/legacycodec.h

To: cfeck, #frameworks, #dolphin, dfaure
Cc: nerdopolist, ngraham, kde-frameworks-devel, michaelh, bruns

D18161: [WIP/RFC] [kioslave/file] Add a codec for legacy filenames

[Christoph Feck]

cfeck created this revision.
cfeck added reviewers: Frameworks, Dolphin.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
cfeck requested review of this revision.

REVISION SUMMARY
  **[WIP/RFC] Please let me know if what I propose is sane**
  
  UNIX filenames can contain any bytes (except `\0` and `/`). Qt's 
`QFile::decodeName()` calls `QString::fromLocal8Bit()`, assuming that all 
filesystems use the system's locale encoding. For filenames that have been 
created with a different encoding, and have not yet been converted (e.g. using 
`convmv`), this creates non-reversible U+FFFD (REPLACEMENT CHARACTER) code 
points in the filenames.
  
  For example, some old-style archives might not contain any information about 
the encoding of the filenames, and even today archivers extract them without 
trying to convert to the locale's encoding.
  
  While full support for those filenames is not needed, Dolphin should at least 
be able to delete, rename, and move those files. Since all actual (local) file 
handling is done inside the `file` kioslave, patching Dolphin will not help.
  
  This code is a near verbatim copy of the code we had in kdelibs, written by 
Szókovács Róbert. Only minor adaptions to Qt5 were done. It decodes invalid 
bytes as U+10FExx from Plane 16 (Supplementary Private Use Area-B) to be able 
to encode them later.
  
  Dolphin could detect filenames with those characters, and either mark them 
(by color or overlay icon), or even automatically offer to rename them.
  
  CCBUG: 204768
  CCBUG: 165044

TEST PLAN
  touch "/tmp/test-"$'\377'".txt"
  dolphin /tmp
  
  Copying and deleting a test file worked with this code, failed without.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D18161

AFFECTED FILES
  src/ioslaves/file/CMakeLists.txt
  src/ioslaves/file/file.cpp
  src/ioslaves/file/legacycodec.cpp
  src/ioslaves/file/legacycodec.h

To: cfeck, #frameworks, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18138: KRatingPainter: Delete copy constructor and assignment operator

[Christoph Feck]

cfeck accepted this revision.
cfeck added a comment.
This revision is now accepted and ready to land.


  Right.
  
  If someone would copy the object using the default implementations, they 
would only get two instances pointing to the same Private.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D18138

To: aacid, cfeck
Cc: cfeck, kde-frameworks-devel, michaelh, ngraham, bruns

D17816: Support for xattrs on kio copy/move

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> copyjob.cpp:1119
> +KJob *job = KIO::copy_xattr((*it).uSource, (*it).uDest);
> +job->exec();
>  //this is required for the undo feature

Here, too.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: cochise, dfaure
Cc: cfeck, bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, 
ngraham, atha.kane, spoorun, nicolasfella, kde-frameworks-devel, michaelh

D17816: Support for xattrs on kio copy/move

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> filecopyjob.cpp:519
> +KJob *job = KIO::copy_xattr(d->m_src, d->m_dest);
> +job->exec();
>  if (d->m_move) {

This waits (i.e. spawns a separate event loop) until the job is finished. 
Should use a subjob.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: cochise, dfaure
Cc: cfeck, bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, 
ngraham, atha.kane, spoorun, nicolasfella, kde-frameworks-devel, michaelh

D17596: [KDirOperator] Allow renaming files from the context menu

[Christoph Feck]

cfeck added a comment.


  What is the status of this patch? It missed the 5.54 deadline, so the version 
would need to be adjusted.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17596

To: ngraham, #frameworks, #dolphin
Cc: cfeck, emateli, elvisangelaccio, markuss, dhaumann, kde-frameworks-devel, 
michaelh, ngraham, bruns

D15573: replace custom backtracing in SlaveBase with KCrash

[Christoph Feck]

cfeck added a comment.


  This seems to pull in QtGui dependency, but that's also dragged in by 
KService (which also links to KF5::Crash) and KF5DBusAddons (which only needs 
QtX11Extras, which unfortunately also needs QtGui).
  
  Can the KCrash dependency be added to the slaves instead of to the KIOCore 
library? The split into Core/Gui/Widgets got a bit void when linking an 
application against KIOCore effectively drags in QtGui and QtWidgets (I have 
yet to check why QtWidgets is dragged in).

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15573

To: sitter, dfaure
Cc: cfeck, kde-frameworks-devel, michaelh, ngraham, bruns

D17907: [KWidgetsAddons] Do not use light font styles for headings (3/3)

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> ktitlewidget.cpp:242
>  d->textLabel->setStyleSheet(d->textStyleSheet());
> -//Qt stylesheet doesn't support lighter font-weight
> -QFont font(d->textLabel->font());
> -if (d->level <= 4) {
> -font.setWeight(QFont::Light);
> -font.setStyleName(QStringLiteral("Light"));
> -} else {
> -font.setWeight(QFont::Normal);
> -font.setStyleName(QStringLiteral("Regular"));
> -}
> +
>  }

Remove empty line before final `}`

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  no-light-headings (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D17907

To: rooty, ngraham, #frameworks, #vdg, #plasma
Cc: cfeck, filipf, kde-frameworks-devel, michaelh, ngraham, bruns

D17828: Don't check twice if the icon exists from ::availableSizes

[Christoph Feck]

cfeck added a comment.


  Maybe add 'const found = ', while you are at it.

REPOSITORY
  R302 KIconThemes

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17828

To: apol, #frameworks, cfeck
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17828: Don't check twice if the icon exists from ::availableSizes

[Christoph Feck]

cfeck accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R302 KIconThemes

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17828

To: apol, #frameworks, cfeck
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17190: Add level api from Kirigami.Heading

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> ktitlewidget.cpp:132
>  titleFrame->setBackgroundRole(QPalette::Base);
> +titleFrame->setContentsMargins(0, 0, 0, 0);
>  

Setting 0 margins effectively disables the frame; the Breeze setting to keep 
frames around page titles is no longer respected.

If this is intended, the frame shouldn't set a background role (and the code 
would have to be checked regarding correct foreground colors).

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D17190

To: mart, #plasma, #vdg, ngraham
Cc: cfeck, ngraham, kde-frameworks-devel, michaelh, bruns