D9987: Lessen log spam by not checking for existence of file with empty name

[Henrik Fehlauer]

rkflx abandoned this revision.
Herald added a subscriber: kde-frameworks-devel.

REPOSITORY
  R303 KInit

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

To: rkflx, #frameworks
Cc: kde-frameworks-devel, dhaumann, michaelh, ngraham, bruns

D12130: Use the more user-friendly string "File type" in the save dialogs

[Henrik Fehlauer]

rkflx resigned from this revision.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #vdg, bruns, alexeymin, abetts
Cc: kde-frameworks-devel, safaalfulaij, davidc, ltoscano, cfeck, rkflx, 
alexeymin, abetts, bruns, michaelh, ngraham

D12545: Set focus on the filename line edit when a file is selected

[Henrik Fehlauer]

rkflx resigned from this revision.
Herald added a subscriber: kde-frameworks-devel.

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg, ngraham
Cc: kde-frameworks-devel, rkflx, ngraham, #frameworks, michaelh, bruns

D12327: Show Detailed Tree View by default

[Henrik Fehlauer]

rkflx resigned from this revision.
Herald added a subscriber: kde-frameworks-devel.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #vdg
Cc: kde-frameworks-devel, elvisangelaccio, abetts, #frameworks, michaelh, 
ngraham, bruns

D12591: KFileWidget: Provide faster access to the icon position setting

[Henrik Fehlauer]

rkflx abandoned this revision.
Herald added a subscriber: kde-frameworks-devel.

REPOSITORY
  R241 KIO

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

To: rkflx, #frameworks, ngraham
Cc: kde-frameworks-devel, ngraham, elvisangelaccio, michaelh, bruns

D7010: KSqueezedTextLabel: call updateGeometry() when text changes

[Henrik Fehlauer]

rkflx resigned from this revision.

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

To: brauch, cfeck
Cc: dhaumann, aacid, #frameworks, michaelh, ngraham, bruns

D12647: Move the inline preview button into the menu

[Henrik Fehlauer]

rkflx resigned from this revision.

REPOSITORY
  R241 KIO

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

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

D14201: Set system default shortcut Ctrl+0 for "Actual Size" action

[Henrik Fehlauer]

rkflx accepted this revision.
rkflx added a comment.


  @muhlenpfordt I'd say go ahead with landing (after updating the version). It 
would be good to still have some testing time before the next Frameworks 
release.
  
  (The new Neon seems to be just around the corner, AFAICS.)

REPOSITORY
  R237 KConfig

BRANCH
  shortcut-actual-size (branched from master)

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

To: muhlenpfordt, dfaure, broulik, ngraham, rkflx
Cc: jriddell, rkflx, kde-frameworks-devel, michaelh, ngraham, bruns

D14662: KPropertiesDialog: switch to label in setFileNameReadOnly(true)

[Henrik Fehlauer]

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

REPOSITORY
  R241 KIO

BRANCH
  kpropsdlg

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

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

D14662: KPropertiesDialog: switch to label in setFileNameReadOnly(true)

[Henrik Fehlauer]

rkflx added a comment.


  Thanks for the help. Works great, just one inline question about an edge case 
(everything else LGTM).

INLINE COMMENTS

> kpropertiesdialog.cpp:853
>  QGridLayout *grid = new QGridLayout(); // unknown rows
> +d->m_grid = grid;
>  grid->setColumnStretch(0, 0);

Just out of interest (don't change anything): I assume you don't fully 
transition everything to `d->m_grid` to keep the diff small, or because `grid` 
is more readable?

> kpropertiesdialog.cpp:1211-1214
> +d->m_fileNameLabel = new QLabel(d->m_frame);
> +
> d->m_fileNameLabel->setTextInteractionFlags(Qt::TextSelectableByMouse | 
> Qt::TextSelectableByKeyboard);
> +d->m_fileNameLabel->setText(d->oldName); // will get overwritten if 
> d->bMultiple
> +d->m_grid->addWidget(d->m_fileNameLabel, 0, 2);

Wouldn't this mean that calling `setFileNameReadOnly(true)` multiple times will 
also create multiple labels on top of each other, breaking idempotence and 
resulting in some kind of bold font effect?

REPOSITORY
  R241 KIO

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

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

D14610: Use KLineEdit for folder name if folder has write access, else use QLabel

[Henrik Fehlauer]

rkflx added a comment.


  In D14610#303987 , @dfaure wrote:
  
  > Hmm, well, for IconApplet's use case
  
  
  I'd say it would be nice to be consistent everywhere.
  
  > I could do it myself, faster than doing 10 reviews :-)
  
  No doubt about that, feel free to work on it ;) If you are busy, we can also 
add this to T9297  so we don't forget about 
it.
  
  ---
  
  In D14610#304109 , @dfaure wrote:
  
  > Make sure test "Create New / Text File" in dolphin.
  
  
  Interesting, for me only Link to Application will bring up the properties 
dialog ;)

REPOSITORY
  R241 KIO

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

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

D14449: Modify device usage information

[Henrik Fehlauer]

rkflx added a comment.


  In D14449#303766 , @shubham wrote:
  
  > In D14449#300327 , @ngraham 
wrote:
  >
  > > +1 for using the same label but putting the information on more than one 
line like @rkflx suggests.
  >
  
  
  As far as I can see that comment was part of the discussion about what to 
show, which I think we now concluded. I don't think that was meant as an advice 
regarding implementation.
  
  You are already creating a new label (which is fine, also where you're doing 
it is the perfect spot). I was only wondering whether the call to `setText` 
should better be placed elsewhere.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Henrik Fehlauer]

rkflx added a comment.


  > This comment has been deleted.
  
  You are doing this in every other Diff, which is a bit annoying. Please first 
check what you wrote, then click on Send. Otherwise people will just ignore 
notifications.
  
  > is there any other simpler way to achieve this?
  
  Maybe, maybe not. I added my idea on that topic above, but it's not my job to 
research that in depth.

INLINE COMMENTS

> shubham wrote in kpropertiesdialog.cpp:1184-1186
> If all function calls to setText are to take place inside 
> slotFreeSpaceResult, then we will need to make a new QLabel, so we can't 
> re-use it(which is the main point as @ngraham had earlier said)

Could you point out where @ngraham said something like that? I cannot find it.

> shubham wrote in kpropertiesdialog.cpp:1279
> so we need to remove setMaximumWidth() ?

Yes.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Henrik Fehlauer]

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


  Thanks, looking better than before now. There are still some improvements you 
could make:
  
  - There is a superfluous space before the comma in the second line.
  - I'd prefer the bar to be a bit wider by default. However, it turns out 
there is a problem with my original suggestion (see inline comment).
  - The vertical spacing between the first and the second line is too big, it 
should be the same as for Size:. However, there should still be enough spacing 
so it also looks good with the Oxygen style. As far as I can see this is an 
issue with how Breeze renders the `KCapacityBar`, in particular the bounding 
rect contains unnecessary margins ([⇧] + [Ctrl]-click on it in GammaRay and 
compare Breeze and Oxygen). Of course that's material for a patch in a 
different repo, but the "hole" in your current screenshot does not look good 
(the second line is closer to the bottom than to the first line, which is 
bad!), and it would be better to fix the problem there before landing the KIO 
patch.

INLINE COMMENTS

> kpropertiesdialog.cpp:1184-1186
> +QStorageInfo *storage = new QStorageInfo(QLatin1String("/"));
> +const quint64 size = storage->bytesTotal();
> +const quint64 free = storage->bytesAvailable();

That's a bit odd: Why would you have to query that information here again, if 
in the original implementation it is already available?

Perhaps all calls to `setText` should take place only in one part of the code, 
e.g. `slotFreeSpaceResult`.

> kpropertiesdialog.cpp:1188
> +l = new QLabel(d->m_frame);
> +grid->addWidget(l, curRow, 2);
> +l->setText(i18nc("Device usage information","%1 used , %2 free", 
> KIO::convertSize(size - free), KIO::convertSize(free)));

Do you need `curRow++` here, in case additional rows are added later on?

> 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)));
> +

Translators will only see the string and the context, so "Device usage 
information" is not enough. It would be better to also describe what the 
parameters are for, see `slotFreeSpaceResult` for an example.

> kpropertiesdialog.cpp:1279
>  
> +d->m_capacityBar->setMaximumWidth(175);
>  d->m_capacityBar->setText(

I don't think we can set a maximum width for the complete bar, because it can 
conflict with languages with longer translations.

More importantly, for the Oxygen widget style the text in a `KCapacityBar` 
might be drawn inline and the bar should span the complete width of the dialog, 
so I don't think we should fiddle too much with the sizes of `m_capacityBar` or 
its sub-components.

I guess we have to live with the longer bar for Breeze, which could already 
become quite long without your patch if you resized the dialog. For other 
languages it is also less of an issue.

> kpropertiesdialog.cpp:1281
>  d->m_capacityBar->setText(
> -i18nc("Available space out of total partition size (percent used)", 
> "%1 free of %2 (%3% used)",
> -  KIO::convertSize(available),
> -  KIO::convertSize(size),
> -  percentUsed));
> +i18nc("Available space out of total partition size (percent used)", 
> "%1% used of %2",
> +  percentUsed,

Please also update the translation context when you change the string to 
translate.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14610: Use KLineEdit for folder name if folder has write access, else use QLabel

[Henrik Fehlauer]

rkflx added a comment.


  In D14610#303537 , @rkflx wrote:
  
  > `KPropertiesDialog::setFileNameReadOnly`
  > `m_bFromTemplate`
  
  
  @shubham Any comments on that (see my questions to @dfaure above)?

INLINE COMMENTS

> kpropertiesdialog.cpp:988
> +KFileItemListProperties itemList(KFileItemList() << item);
> +if (d->bMultiple || isTrash || hasRoot || !itemList.supportsMoving() || 
> (itemList.isDirectory() & !itemList.supportsWriting())) {
>  QLabel *lab = new QLabel(d->m_frame);

Without your patch, we only tested for `!itemList.supportsMoving()`. Now you 
also test for `(itemList.isDirectory() & !itemList.supportsWriting()`. Could 
you explain in what cases we need that new test? As far as I can see this 
blocks renaming the folder you removed the write permission from (while only 
its children should be prevented from being renamed).

(`&` instead of `&&` also caught my eye.)

> kpropertiesdialog.cpp:999-1019
> +d->m_lined = new KLineEdit(d->m_frame);
> +d->m_lined->setObjectName("KFilePropsPlugin::nameLineEdit");
> +d->m_lined->setText(filename);
> +d->nameArea = d->m_lined;
> +d->m_lined->setFocus();
> +grid->addWidget(d->nameArea, curRow++, 2);
> +

Please don't add additional indentation here.

> kpropertiesdialog.cpp:1021
>  
> -grid->addWidget(d->nameArea, curRow++, 2);
> -

You are moving that line to both the `if` and the `else` branch. Why not keep 
it here?

REPOSITORY
  R241 KIO

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

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

D14449: Modify device usage information

[Henrik Fehlauer]

rkflx added a comment.


  In D14449#303660 , @shubham wrote:
  
  > > Let me plug my suggestion again:
  > > 
  > >   Device capacity: ==- 24% used of 94.4 Gib
  > >22.5 GiB used, 71.9 GiB free
  > >
  >
  > if this design is used , then the capacity bar would look elongated and 
stretched, which doesn't looks good.
  
  
  Not if you set a maximum width.
  
  > Since used space is not of that much importance, so we can just omit it 
like this
  >  Device capacity: ===> 24% full of 90GiB (70GiB free)
  
  That's more or less what we currently have without your patch, but if we want 
to close the bug as WONTFIX I'd prefer the original order.

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14610: Use KLineEdit for folder name if folder has write access, else use QLabel

[Henrik Fehlauer]

rkflx added a comment.


  @shubham Thanks for helping out with T9297 
!
  
  > reuse the above code that creates a QLabel
  
  @dfaure Thanks for the review! Any advice on how to handle 
`KPropertiesDialog::setFileNameReadOnly`, which is used in Plasma's 
`IconApplet` 

 and would still show the `KLineEdit` if I understand your proposal correctly?
  
  Also, `KFilePropsPlugin::setFileNameReadOnly` checks for `m_bFromTemplate`. 
Would this be relevant for your suggestion too?
  
  > the patch description is unclear
  
  The main motivation for the patch is to also indicate visually that an item's 
name cannot be changed. Currently users only notice that by chance due to 
typing into the line edit not being allowed.

REPOSITORY
  R241 KIO

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

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

D14501: Top-align labels in properties dialog

[Henrik Fehlauer]

rkflx added a comment.


  Just realized this actually fixes a problem present in KDE 3 already (KDE 2 
did not have the second line yet, as far as I could tell in a short but very 
interesting test).
  
  In D14501#301044 , @cfeck wrote:
  
  > Bonus points if you submit another patch that addresses the issue where it 
starts out with only one text line, then (after computing the size), jumps to a 
two-line layout.
  >
  > On remote folders, it looks even worse, because the text line is empty, and 
so the "Calculate" button looks disconnected from the Size label.
  
  
  True, but that requires a bit more work than a simple drive-by patch ;) For 
now, I created T9297: Polish file/folder properties dialog 
 for coordination.

REPOSITORY
  R241 KIO

BRANCH
  kpropertiesdialog-alignment-fix (branched from master)

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

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

D14501: Top-align labels in properties dialog

[Henrik Fehlauer]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:9829d0ef9e74: Top-align labels in properties dialog 
(authored by rkflx).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14501?vs=38811=38865

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

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp

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

D14501: Top-align labels in properties dialog

[Henrik Fehlauer]

rkflx created this revision.
rkflx added reviewers: dhaumann, ngraham.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
rkflx requested review of this revision.

REVISION SUMMARY
  The General tab of KIO's properties dialog shows labels on the
  left side, and properties on the right, creating a table-like structure.
  Commonly a label should be aligned vertically in such a way it matches
  the first line of the properties, to make it clear which properties it
  belongs to.
  
  However, this was not yet the case for the Size label.

TEST PLAN
  In Dolphin, select a folder and press [Alt] + [Return]. All labels in
  Properties > General should be top-aligned.
  Before: F6165126: kio-properties-before.png 

  After: F6165125: kio-properties-after.png 


REPOSITORY
  R241 KIO

BRANCH
  kpropertiesdialog-alignment-fix (branched from master)

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

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp

To: rkflx, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Henrik Fehlauer]

rkflx added a comment.


  In D14449#300841 , @dhaumann wrote:
  
  >   Device capacity: 94.4 Gib
  >   Device usage:==-- 24% full (71.9 GiB free, 22.5 GiB used)
  >   
  >
  > Could you give this a try with screenshot?
  
  
  I'm afraid the second line will become too long for locales other than 
English, making the capacity bar a tiny dot. Try it in GammaRay ([⇧] + [Ctrl] 
click, then change the text, add some spacing in front which currently is 
missing).
  
  Let me plug my suggestion again:
  
Device capacity: ==- 24% used of 94.4 Gib
 22.5 GiB used, 71.9 GiB free
  
  @ngraham Any opinions on that?

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Display used space in GiB also

[Henrik Fehlauer]

rkflx added a comment.


  In D14449#300327 , @ngraham wrote:
  
  > Here's another idea for how to express everything while remaining compact 
in the horizontal dimension:
  >
  >   Device capacity: -- 24% full
  >22.5 GiB used, 71.9 GiB free
  >94.4 Gib total size
  
  
  Not bad at all, but due to using 3 lines now puts quite some emphasis on the 
device, while the dialog is about the folder actually. Would this be a 
compromise?:
  
Device capacity: ==- 24% used of 94.4 Gib
 22.5 GiB used, 71.9 GiB free
  
  (Not too happy about the duplication of "used", though.)

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Display used space in GiB also

[Henrik Fehlauer]

rkflx added a comment.


  In D14449#300086 , @ngraham wrote:
  
  >   Device capacity: 94.4 Gib
  >   Device usage:==-- 24% full (71.9 GiB free, 22.5 GiB used)
  >
  
  
  That's much better. Another proposal built on top of that:
  
Device capacity: 94.4 Gib (22.5 GiB used, 71.9 GiB free)
 -- 24% full 
  
  (Does not duplicate "device", shorter for improved translatability, same 
units on the same line, order of used/free matches what the bar shows.)
  
  Or perhaps there is a way to emphasize the most important information a bit 
more (IMO that's "71.9 GiB free")?

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14201: Set system default shortcut Ctrl+0 for "Actual Size" action

[Henrik Fehlauer]

rkflx added a comment.


  Thanks for your support ;)
  
  In D14201#296520 , @ngraham wrote:
  
  > Let's hold off on landing this patch (if accepted by everyone) until  
Cirkuit has had a release with D14202  so 
we don't wind up with the dreaded "Ambiguous Shortcut!" dialog.
  
  
  Are you sure about that? This might mean waiting forever.
  
  AFAIK Cirkuit did not even have a KF5 release yet, I'd assume everyone is 
just running from the `frameworks` branch, or stuck on an old KDE4 distro which 
would not get the new KConfig anyway. Ubuntu shipped it in 16.04, 18.04 does 
not have it anymore in the default repos. This app is not exactly a second 
DigiKam where your comment might be coming from, with wide distribution and 
regular releases.

REPOSITORY
  R237 KConfig

BRANCH
  shortcut-actual-size (branched from master)

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

To: muhlenpfordt, dfaure, broulik, ngraham
Cc: rkflx, kde-frameworks-devel, michaelh, ngraham, bruns

D14201: Set system default shortcut Ctrl+0 for "Actual Size" action

[Henrik Fehlauer]

rkflx added a comment.


  @ngraham Would this patch be something you could back?

REPOSITORY
  R237 KConfig

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

To: muhlenpfordt, dfaure, broulik, ngraham
Cc: rkflx, kde-frameworks-devel, michaelh, ngraham, bruns

D14161: [KStyle] Use dialog-question for question icon

[Henrik Fehlauer]

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


  Thanks for noticing, LGTM and restores the proper icon indeed.
  
  > This was changed in `KMessageBox` in 
9b05a12a43dbe2b31a4655052f20de0832ac7a61 
 but 
never "upstreamed" to `KStyle`
  
  Sorry, did not know about this, and neither did the reviewer notice.
  
  Should I submit Diffs (or commit directly?) where the wrong icon is still in 
use, e.g. here 
, 
here 
, 
here 

 and/or here 
?

REPOSITORY
  R252 Framework Integration

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

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

D14160: [KMessageBox] Call style for icon

[Henrik Fehlauer]

rkflx added a comment.


  > Dialog icons are correct on Linux with Breeze
  
  …if you also apply D14161 . Otherwise 
LGTM.
  
  > This needs testing on Windows and Mac
  
  You could add or ping more reviewers who happen to have a dev setup on those 
platforms, perhaps #Elisa ?

REPOSITORY
  R236 KWidgetsAddons

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

To: broulik, dfaure, kfunk, dhaumann
Cc: rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D12218: Remove Reload button from the file dialogs' toolbar

[Henrik Fehlauer]

rkflx added a comment.


  In D12218#271290 , @davidedmundson 
wrote:
  
  > > It can be useful for network views, but those are a minority of use cases
  >
  > Every large scale KDE deployment (from my POV by far our most important 
userbase) has a network share.
  >
  > It might not be needed often, but when it's needed it's needed.
  
  
  Would you be okay with some of the compromises mentioned above, or is this 
again an all-or-nothing thing?
  
  - Show toolbar button only for network folders.
  - Move Reload to `KUrlNavigator`.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, abetts, markg, broulik, rkflx, 
#dolphin, michaelh, ngraham, bruns

D12756: [KDateTable] Use more appropriate and readable text colors for weekends and holidays

[Henrik Fehlauer]

rkflx added a comment.


  In D12756#268345 , @ngraham wrote:
  
  > Use conditional grays instead of reds (red was never an appropriate color 
anyway)
  
  
  Hm, now Sundays look like they belong to the previous/next month. (Do we even 
need to change colour everywhere, or would it be enough to only highlight the 
weekend in the header?)
  
  You could also look at the way KDE3 styled this (see 
https://edu.kde.org/kstars/feature/SetTime.png).

REPOSITORY
  R236 KWidgetsAddons

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

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

D12986: [Trash KCM] Clean up and standardize UI to be in line with the KDE HIG

[Henrik Fehlauer]

rkflx added a comment.


  In my personal experience spending more time on a limited set of items leads 
to better results and less churn (as in reverting commits, number of iterations 
per patch etc.) than only lightly touching a large amount of issues everywhere. 
Of course you can do whatever you feel is right, I was merely hinting at a 
potential issue.

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, #frameworks, #vdg
Cc: abetts, rkflx, kfm-devel, kde-frameworks-devel, michaelh, ngraham, bruns

D12858: [KCharSelect] Fix table cell size

[Henrik Fehlauer]

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


  Thanks, works great now!

REPOSITORY
  R236 KWidgetsAddons

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

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

D12986: [Trash KCM] Clean up and standardize UI to be in line with the KDE HIG

[Henrik Fehlauer]

rkflx added a comment.


  > The value is in standardizing on something.
  
  Personally I'd be okay with a consistent style per application, with the new 
style applied to new applications, and allowing exceptions where it makes 
sense. You've already witnessed the amount of bikeshedding in Gwenview when we 
needed to add options, because with the new style it is much more challenging 
to get the wording right than with the traditional style.
  
  Anyway, I'm not complaining too much (but please be aware that every change 
you propose creates additional work for reviewers and translators).

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, #frameworks, #vdg
Cc: abetts, rkflx, kfm-devel, kde-frameworks-devel, michaelh, ngraham, bruns

D12130: Use the more user-friendly string "File type" in the save dialogs

[Henrik Fehlauer]

rkflx added a comment.


  @ngraham I guess you are still working on this, or did you move on to other 
things in the meantime? (And please set the status to "Changes planned", so 
this does not clog up the review queue).

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #vdg, bruns, alexeymin, rkflx, abetts
Cc: kde-frameworks-devel, safaalfulaij, davidc, ltoscano, cfeck, rkflx, 
alexeymin, abetts, bruns, michaelh, ngraham

D12986: [Trash KCM] Clean up and standardize UI to be in line with the KDE HIG

[Henrik Fehlauer]

rkflx added a comment.


  In D12986#265666 , @ngraham wrote:
  
  > In D12986#265662 , @rkflx wrote:
  >
  > > Anyway, I'm not complaining too much (but please be aware that every 
change you propose creates additional work for reviewers and translators).
  >
  >
  > I'm quite aware, but this is just the nature of submitting patches, right? 
Nobody's making you review mine, and I hope you don't feel like it's your 
responsibility to babysit me to prevent things from getting broken or anything 
like that. :)
  
  
  Well, I'd say you should use the resources available (which are quite scarce, 
as you are aware) wisely. I'd rather have the translators translate strings in 
Gwenview (there are many missing spots where we added things for 18.04) and 
work on my review backlog. Also note that fighting an uphill battle, i.e. more 
stuff getting broken than you are able to fix by yourself, is not much fun.

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, #frameworks, #vdg
Cc: abetts, rkflx, kfm-devel, kde-frameworks-devel, michaelh, ngraham, bruns

D12986: [Trash KCM] Clean up and standardize UI to be in line with the KDE HIG

[Henrik Fehlauer]

rkflx added a comment.


  Thanks, "Size" and "Full trash" are looking good now.
  
  In D12986#265235 , @rkflx wrote:
  
  > In D12986#265231 , @ngraham 
wrote:
  >
  > > Yeah, that makes sense. Suggestions welcome, of course. Is this any 
better?
  > >  F5859259: After 2.png 
  >
  >
  > Hm, I think after the checkbox there should some sort of sentence, with a 
noun in front:
  >
  >   Cleanup: [ ] Automatically empty after 7 days
  
  
  Just noticed that I based my suggestion on your screenshot, but missed to see 
that you changed the semantics compared to the original dialog: The trash won't 
be emptied completely after 7 days, but files older than 7 days will be deleted 
(based on the wording, you might want to check the code what's correct).
  
  Therefore I think we might have to keep the original wording and only add the 
label in front.
  
  (Which is why I'm not too fond of changing every UI to the new style, because 
it is much work but creates little value.)
  
  What's your plan for landing this change, do you want to convert the other 
pages first?

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, #frameworks, #vdg
Cc: abetts, rkflx, kfm-devel, kde-frameworks-devel, michaelh, ngraham, bruns

D12858: [KCharSelect] Fix table cell width

[Henrik Fehlauer]

rkflx added a comment.


  Not sure whether that's material for this patch, but for me the vertical 
spacing is still a bit off, i.e. the boxes are still not square as before:
  
  Qt 5.10:
  F5859388: kcharselect-qt5.10.png 
  
  Qt 5.11:
  F5859390: kcharselect-qt5.11.png 
  
  Qt 5.11 (patch applied):
  F5859392: kcharselect-qt5.11-patched.png 

  
  (Spinbox issue already reported by @cfeck in 
https://bugreports.qt.io/browse/QTBUG-68238…)

REPOSITORY
  R236 KWidgetsAddons

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

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

D12986: [Trash KCM] Clean up and standardize UI to be in line with the KDE HIG

[Henrik Fehlauer]

rkflx added a comment.


  In D12986#265231 , @ngraham wrote:
  
  > Yeah, that makes sense. Suggestions welcome, of course. Is this any better?
  >  F5859259: After 2.png 
  
  
  Hm, I think after the checkbox there should some sort of sentence, with a 
noun in front:
  
Cleanup: [ ] Automatically empty after 7 days

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, #frameworks, #vdg
Cc: abetts, rkflx, kfm-devel, kde-frameworks-devel, michaelh, ngraham, bruns

D12986: [Trash KCM] Clean up and standardize UI to be in line with the KDE HIG

[Henrik Fehlauer]

rkflx added a comment.


  I don't think putting whole phrases as a label before the checkbox works very 
well, and I would be surprised if that's what the HIG now recommends. I assume 
at most a single subject should be used there?
  
  For example, instead of the awkwardly placed checkbox in
  
Limit to maximum size: [ ] 10 %
  
  …it would work much better this way:
  
Size: [ ] Limit to 10 %
  
  Not only is scanning the dialog much quicker if you only have to read a 
single word when looking for an option in that vertical row, but for languages 
with longer translations it's the only way to make the dialog not fall apart in 
the horizontal direction.
  
  (Same reasoning applies to the other options too, of course.)

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, #frameworks, #vdg
Cc: rkflx, kfm-devel, kde-frameworks-devel, michaelh, ngraham, bruns

D12337: Give the file dialogs a "Sort by" menu button on the toolbar

[Henrik Fehlauer]

rkflx added a comment.


  Let's commit this on a separate branch, as getting micro-approvals from 
#Frameworks  does not work that 
well. Then we can implement (in the form of reviewed Diffs as before) what we 
already discussed in a bigger setting, e.g.
  
  - rework the view modes
  - set new defaults
  - put the New folder button on the bottom
  - decide on D12647: Move the inline preview button into the menu 

  - figure out D12218: Remove Reload button from the file dialogs' toolbar 

  - figure out Up
  - hopefully abandon D12333: Put the open/save dialog's toolbar above all 
other widgets, like Dolphin does  for good
  
  …and finally present the final outcome to a wider audience for approval 
before merging. This way can we make progress as time allows, without shipping 
incomplete features or get bogged down with dependencies. Note I'm not 
proposing to wait for //everything// you put into T8552 
.
  
  (Sorry for not being able to help more currently, but I first wanna reduce my 
Gwenview and Spectacle backlogs…)

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D12337

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

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: kde-frameworks-devel, andreaska, markg, broulik, anemeth, michaelh, 
ngraham, bruns

D12734: Scale up folder icon before creating preview overlays

[Henrik Fehlauer]

rkflx added a comment.


  In D12734#262368 , @muhlenpfordt 
wrote:
  
  > Ok to commit?
  
  
  Sure, go ahead 

REPOSITORY
  R320 KIO Extras

BRANCH
  scale-up-folder-icon (branched from Applications/18.04)

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

To: muhlenpfordt, #frameworks, rkflx
Cc: ngraham, rkflx

D12848: Set fix steps for icon sizes

[Henrik Fehlauer]

rkflx added a comment.


  > In the open/save dialog use the same fixed icon sizes for the zoom slider 
as Dolphin.
  
  So we could also change Dolphin to support continuous zoom instead? As the 
summary is quite sparse, I'm not sure what the patch is about: Is it about 
consistency, or is there a fundamental reason why fixed sizes are better for 
users?
  
  Note that we allow arbitrary window sizes, sidebar sizes and so on. Targeting 
a specific icon size can still easily be done by clicking on the button, or by 
dragging the slider while observing the tooltip showing the pixel size.
  
  Personally I prefer smooth zooming (which Dolphin is doing, too) instead of 
having big jumps between steps. Nevertheless, if you can find more +1's and 
better reasons for the change itself, I'm not going to block this patch. 
However, "nobody complained for Dolphin" is not good enough of a reason.
  
  Could you research why for Dolphin there are discrete sizes? Maybe this stems 
from pre-SVG times, where only certain pixel sizes had sharp rendering of 
icons. If this is the case, with today's SVGs we could easily support all 
in-between sizes without much loss of rendering quality.
  
  In D12848#262303 , @ngraham wrote:
  
  > IMHO what we really need is a standard size slider widget so that each app 
doesn't need to reinvent the wheel. Right now Gwenview, Dolphin, and the file 
dialogs each implement their own, and they're all subtly (or very) different 
from each other.
  >
  > That said, I find myself agreeing with Kai and Alex here. Arbitrary zoom 
levels aren't really needed in the file dialog like they are in Gwenview. 
Dolphin's slider has discrete sizes and I don't think it causes problems there.
  
  
  I'm confused: First you propose a common widget, and then you say Gwenview 
needs arbitrary zoom levels?

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg
Cc: broulik, rkflx, ngraham, kde-frameworks-devel, michaelh, bruns

D12848: Set fix steps for icon sizes

[Henrik Fehlauer]

rkflx added a comment.


  In D12848#262008 , @broulik wrote:
  
  > I would vote for removing the buttons
  
  
  We already discussed this and decided to keep the buttons.

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg
Cc: broulik, rkflx, ngraham, kde-frameworks-devel, michaelh, bruns

D12130: Use the more user-friendly string "File type" in the save dialogs

[Henrik Fehlauer]

rkflx added a comment.
Restricted Application added a subscriber: kde-frameworks-devel.


  In D12130#250478 , @ngraham wrote:
  
  > Yes, and I did more investigation myself, and all the glue is in 
plasma-integration. Some changes might be needed there too (or maybe //all// 
the changes will be needed there, we'll see!)
  
  
  Are you sure about the relation to `plasma-integration`? To me it seems that 
the widget can also be used standalone. Also, in KDE4 `plasma-integration` was 
not even there, yet the string was shown in the UI, and different types of 
dialogs were possible. My guess would be that this can be solved entirely in 
`KIO`, perhaps by following code paths relating to `selectMimeTypeFilter`.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #vdg, bruns, alexeymin, rkflx, abetts
Cc: kde-frameworks-devel, safaalfulaij, davidc, ltoscano, cfeck, rkflx, 
alexeymin, abetts, bruns, michaelh, ngraham

D12848: Set fix steps for icon sizes

[Henrik Fehlauer]

rkflx added a comment.


  As mentioned in the task:
  
  In T8552#139581 , @rkflx wrote:
  
  > > In T8552#139578 , @anemeth 
wrote:
  > > 
  > >> Could we have the same icon zoom slider as in Dolphin?
  > >>
  > >> - Predefined sizes only, no arbitary icon sizes
  >
  > Hm, OTOH if we keep the buttons, having predefined sizes in the slider is 
not needed anymore… Better keep the fine-grained control for those that want 
and need it. Removing that could be disliked by some.

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg
Cc: rkflx, ngraham, kde-frameworks-devel, michaelh, bruns

D12538: Allow accepting by double-click in save dialog

[Henrik Fehlauer]

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

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D12538 (branched from master)

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

To: anemeth, #frameworks, #vdg, ngraham, rkflx
Cc: kde-frameworks-devel, ltoscano, rkflx, broulik, jtamate, ngraham, 
#frameworks, michaelh, bruns

D12538: Allow accepting by double-click in save dialog

[Henrik Fehlauer]

rkflx added inline comments.

INLINE COMMENTS

> ngraham wrote in kfilewidget.cpp:1866
> Not for everything, just this one.

Well, how should that line look like then?

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg, ngraham, rkflx
Cc: kde-frameworks-devel, ltoscano, rkflx, broulik, jtamate, ngraham, 
#frameworks, michaelh, bruns

D12538: Allow accepting by double-click in save dialog

[Henrik Fehlauer]

rkflx added inline comments.

INLINE COMMENTS

> ngraham wrote in kfilewidget.cpp:1866
> Let's use new-style signal/slot syntax for new code. See 
> https://wiki.qt.io/New_Signal_Slot_Syntax

Is this even possible without a lot of refactoring (which would be out-of-scope 
for this patch)?

> kfilewidget.cpp:2183
> +// double clicking to save should only work on files
> +QFileInfo inf(ops->selectedItems().urlList().first().path());
> +

I find `inf` as a variable name slightly too cryptic.

But actually I'd just write this inline and go without `QFileInfo` and the 
`QUrl` conversion altogether, because `KFileItem` should already provide what 
is needed:

  && ops->selectedItems().first().isFile()

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg, ngraham, rkflx
Cc: kde-frameworks-devel, ltoscano, rkflx, broulik, jtamate, ngraham, 
#frameworks, michaelh, bruns

D12734: Scale up folder icon before creating preview overlays

[Henrik Fehlauer]

rkflx added a comment.


  In D12734#259445 , @muhlenpfordt 
wrote:
  
  > I wanted to be careful here since I'm a bit afraid of changing library 
functions. If you think it's ok for `Applications/18.04` I'll rebase it.
  
  
  How else would you fix problems in a library ;)
  
  Nevertheless, let's check the impact of this again: 
https://lxr.kde.org/ident?_i=thumbForDirectory
  
  Does not look too critical to me, and we probably tested most of the code 
paths.

REPOSITORY
  R320 KIO Extras

BRANCH
  master

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

To: muhlenpfordt, #frameworks, rkflx
Cc: ngraham, rkflx

D12734: Scale up folder icon before creating preview overlays

[Henrik Fehlauer]

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


  +1, but I'd feel a bit uncomfortable (almost) self-approving my own patch 
(sans the `qMax`) here, so perhaps wait until the end of the week for any other 
opinions before committing.
  
  BTW, any reason you are targetting `master` only?

REPOSITORY
  R320 KIO Extras

BRANCH
  master

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

To: muhlenpfordt, #frameworks, rkflx
Cc: ngraham, rkflx

D12077: Show view mode buttons in the open/save dialog's toolbar

[Henrik Fehlauer]

rkflx added a comment.


  One more idea: Would it make sense to land each part of the rework on a 
dedicated rework branch first, and only once everything fits together (both in 
design and implementation) merge that branch to master?
  
  This would avoid huge dependency stacks as well as the need to rush for 
deadlines in the release schedule.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, rkflx, #vdg, abetts
Cc: elvisangelaccio, sharvey, rkflx, mmustac, broulik, michaelh, ngraham, bruns

D12337: Give the file dialogs a "Sort by" menu button on the toolbar

[Henrik Fehlauer]

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


  Thanks, LGTM now.

INLINE COMMENTS

> ngraham wrote in kdiroperator.cpp:1888
> The problem conceptually is that `view-sort-ascending` is semantically 
> inaccurate for anything but ascending order. We don't actually have an icon 
> yet that means "general sort options are here!" That's covered by 
> https://bugs.kde.org/show_bug.cgi?id=393318, and I've pinged Andreas again. 
> No matter what flawed we choose as a placeholder, I'm going to wait for the 
> better icon before landing this, so for now let's just leave it the way it is.

I think it is a misconception that toolbar icons represent state. I don't know 
of any toolbar in our software where this is the case, so why should users 
suddenly expect that what's on the icon represents exactly what is happening, 
e.g. A-Z ascending? It is merely an example of what type of actions they can 
expect when clicking on the button.

Icons are a symbolic representation of general concept, not a literal display 
of a specific state.

---

Anyway, if you want block everything on that, that's what we'll do.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D12337

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

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

D12337: Give the file dialogs a "Sort by" menu button on the toolbar

[Henrik Fehlauer]

rkflx added inline comments.

INLINE COMMENTS

> kdiroperator.cpp:1888
>  d->actionCollection->addAction(QStringLiteral("sorting menu"),  
> sortMenu);
> +sortMenu->setIcon(QIcon::fromTheme(QStringLiteral("itemize")));
>  

While we agreed upon wanting a better icon, until that's done I'd prefer 
`view-sort-ascending` instead. For me, `itemize` has no connection to sorting 
at all, sorry.

I'm aware my alternative shows a specific mode, but TBH I don't think users 
will be put off too much by this detail, in particular because it is the only 
sorting-related icon in the dialog.

Anyway, that's just my preference. Let me know if you think `itemize` is vastly 
better.

> kfilewidget.cpp:365
>  opsWidgetLayout->setSpacing(0);
> -//d->toolbar = new KToolBar(this, true);
> -d->toolbar = new KToolBar(d->opsWidget, true);
> +d->toolbar = new KToolBar(this, true);
>  d->toolbar->setObjectName(QStringLiteral("KFileWidget::toolbar"));

?

> kfilewidget.cpp:365-369
> -//d->toolbar = new KToolBar(this, true);
> -d->toolbar = new KToolBar(d->opsWidget, true);
> +d->toolbar = new KToolBar(this, true);
>  d->toolbar->setObjectName(QStringLiteral("KFileWidget::toolbar"));
>  d->toolbar->setMovable(false);
> -opsWidgetLayout->addWidget(d->toolbar);

?

> kfilewidget.cpp:554-559
> +// Tweak the look and feel of the sort menu button
> +foreach(QToolButton* button, d->toolbar->findChildren()) {
> +if (button->defaultAction() == coll->action(QStringLiteral("sorting 
> menu"))) {
> +button->setPopupMode(QToolButton::InstantPopup);
> +}
> +}

This also worked for me, and would avoid the `foreach`:

  KActionMenu *x = new 
KActionMenu(QIcon::fromTheme(QStringLiteral("configure")), i18n("Options"), 
this);
  x->setMenu(coll->action(QStringLiteral("sorting menu"))->menu());
  x->setDelayed(false);
  d->toolbar->addAction(x);

> kfilewidget.cpp:561
> +
> +
>  KUrlCompletion *pathCompletionObj = new 
> KUrlCompletion(KUrlCompletion::DirCompletion);

Unintentional newline?

> kfilewidget.cpp:1410
>  boxLayout->setMargin(0); // no additional margin to the already existing
> +boxLayout->addWidget(toolbar);
>  

?

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

D12647: Move the inline preview button into the menu

[Henrik Fehlauer]

rkflx added a comment.


  In T8552#140332 , @rkflx wrote:
  
  > > moving the Show Preview button to the menu
  >
  > Makes sense to me, but might be controversial with others.
  
  
  FWIW, if people do not like this (e.g. because they often toggle previews, or 
because it should look consistent with Dolphin), I'd also be fine with keeping 
the button if in the end it turns out we have the space for it.
  
  Keeping the button would also allow to show the tooltip text, explaining why 
the action sometimes is disabled. Having no such explanation for the menu item 
feels a bit awkward.
  
  Patch itself LGTM, but needs more +1's on the idea.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, rkflx
Cc: anemeth, michaelh, bruns

D12337: Give the file dialogs a "Sort by" menu button on the toolbar

[Henrik Fehlauer]

rkflx added a comment.


  In D12337#256813 , @ngraham wrote:
  
  > the context menu will have "Sorting > Sort by name. Thoughts?
  
  
  Actually I'd still find that kind of acceptable.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

D12337: Give the file dialogs a "Sort by" menu button on the toolbar

[Henrik Fehlauer]

rkflx added a comment.


  In D12337#256813 , @ngraham wrote:
  
  > Now we have a problem. With no text in the toolbar button, the actual items 
don't reveal their purpose so readily unless we change all of their strings to 
say "Sort by Name" (etc.). But if we do that, then the context menu will have 
"Sorting > Sort by name. Thoughts?
  
  
  What about injecting a header as proposed in D12337#249821 
?
  
  Or could we simply remove the context menu (only for the file dialog)?

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#256797 , @anemeth wrote:
  
  > In D12321#256791 , @ngraham 
wrote:
  >
  > > Reverted. Alex, now that you have a fancy contributor account, you can 
land this yourself on 5/6/18, or else offer up a version of this without the 
tooltip change and petition to get that in for 5.46.
  >
  >
  > I'll rather wait.
  >  5/6 as in May 6th or June 5th?
  
  
  Please read the link I gave and understand what a string freeze is. As you 
have commit access now, that's your job to get right ;)

REPOSITORY
  R241 KIO

BRANCH
  conditional_preview (branched from master)

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#256786 , @ngraham wrote:
  
  > Darn, my bad. Could we revert just the tooltip string change and land that 
for 5.47, or would you prefer to hold the whole feature until 5.47?
  
  
  Well, the tooltip was added for a reason. The reason is still there, so the 
tooltip still has to be there.
  
  Maybe later on we won't need the tooltip when removing the button, but until 
that's done (and agreed upon – I did not see any agreement from others, yet you 
already started on the implementation…), it should stay. Let's not rush this 
please, it only leads to more work in the end.

REPOSITORY
  R241 KIO

BRANCH
  conditional_preview (branched from master)

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#253903 , @rkflx wrote:
  
  > Cool, LGTM now. Due to the string change this will have to wait for 5.47
  
  
  ...and I even added a warning :/

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  This breaks the string freeze. Please revert.
  
  See https://community.kde.org/Schedules/Frameworks.

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12594: KFileWidget: Perfectly align filename widget with icon view

[Henrik Fehlauer]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:d5f67b07b5ce: KFileWidget: Perfectly align filename 
widget with icon view (authored by rkflx).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12594?vs=33276=33448

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: rkflx, #frameworks, ngraham
Cc: ngraham, elvisangelaccio, michaelh, bruns

D12593: KFileWidget: Save places panel width also after hiding panel

[Henrik Fehlauer]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:0bcc7eadf5ca: KFileWidget: Save places panel width also 
after hiding panel (authored by rkflx).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12593?vs=33275=33447

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: rkflx, #frameworks, ngraham
Cc: ngraham, elvisangelaccio, michaelh, bruns

D12592: KFileWidget: Prevent places panel width from growing 1px iteratively

[Henrik Fehlauer]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:302c70b39723: KFileWidget: Prevent places panel width 
from growing 1px iteratively (authored by rkflx).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12592?vs=33274=33446

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: rkflx, #frameworks, ngraham
Cc: ngraham, elvisangelaccio, michaelh, bruns

D12590: KFileWidget: Disable zoom buttons once reached minimum or maximum

[Henrik Fehlauer]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:00d8a78f5696: KFileWidget: Disable zoom buttons once 
reached minimum or maximum (authored by rkflx).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12590?vs=33367=33443

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: rkflx, #frameworks, ngraham
Cc: ngraham, michaelh, bruns

D12588: KFileWidget: Set minimum size for zoom slider

[Henrik Fehlauer]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:e4f4c66f96d8: KFileWidget: Set minimum size for zoom 
slider (authored by rkflx).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12588?vs=33271=33442

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: rkflx, #frameworks, ngraham
Cc: ngraham, michaelh, bruns

D12545: Set focus on the filename line edit when a file is selected

[Henrik Fehlauer]

rkflx added a comment.


  In D12545#256043 , @anemeth wrote:
  
  > I'm thinking about abandoning this revision in favor of proper tab ordering 
instead.
  
  
  As Nate already said, tabstop ordering would also be good to have. But given 
that the feature already works for single-click users, maybe you could make it 
work for double-click users too (i.e. for the first "selection" click) instead 
of abandoning? After all, in D12538  we are 
doing something similar (add a double-click feature for single-click users).

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg, ngraham, rkflx
Cc: rkflx, ngraham, #frameworks, michaelh, bruns

D12077: Show view mode buttons in the open/save dialog's toolbar

[Henrik Fehlauer]

rkflx added a comment.


  I wonder if we should revert this before it ships to users. I'd rather not 
introduce this in 5.46 if we are about to change it again for 5.47.
  
  Better accumulate visual changes a bit, because constantly changing the 
dialog might get annoying for users.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, rkflx, #vdg, abetts
Cc: sharvey, rkflx, mmustac, broulik, michaelh, bruns

D12590: KFileWidget: Disable zoom buttons once reached minimum or maximum

[Henrik Fehlauer]

rkflx updated this revision to Diff 33367.
rkflx added a comment.


  - Rename actions too

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12590?vs=33272=33367

BRANCH
  disable-zoom-buttons (branched from master)

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: rkflx, #frameworks
Cc: ngraham, michaelh, bruns

D12591: KFileWidget: Provide faster access to the icon position setting

[Henrik Fehlauer]

rkflx planned changes to this revision.
rkflx added a comment.


  In D12591#255639 , 
@elvisangelaccio wrote:
  
  > > While the submenu itself is not advertised in the API docs, it was 
publicly accessible
  >
  > Where? Isn't it a member of `KDirOperator::Private`? (I'm referring to 
`decorationMenu`)
  
  
  That was my first thought too, but unfortunately it leaks out via the 
`actionCollection()`. See my patch to K3b (D12598 
), where it was used that way. I'd rather 
not break third-party users I don't know about…
  
  ---
  
  In D12591#255732 , @ngraham wrote:
  
  > One thought: with this patch, there will be two disabled and inapplicable 
items in the menu when you're not using Short View, which might confuse people. 
Might be appropriate to only show them for Short View.
  
  
  Isn't not confusing people what the "disabled state" is there for, compared 
to making widgets invisible? At least that's how it's done in most other 
places: Disable options which don't apply. Granted, the connection to Short 
View is a bit hard to discover, but that was the case before the patch too 
(even more so).
  
  Nevertheless, I'll hold of with the patch for now. Another more radical way 
forward would be to merge Next and Above as two separate View modes like in 
Dolphin (and get rid of the rest?). Seems like we need more discussion about 
the modes in general, let's do that in T8552 
.

REPOSITORY
  R241 KIO

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

To: rkflx, #frameworks, ngraham
Cc: ngraham, elvisangelaccio, michaelh, bruns

D12538: Allow accepting by double-click in save dialog

[Henrik Fehlauer]

rkflx added a comment.


  In D12538#255954 , @anemeth wrote:
  
  > In D12538#254871 , @rkflx wrote:
  >
  > > However, for double click mode and Save, descending into directories is 
kinda broken. You might want to fix that before shipping…
  >
  >
  > I can't reproduce this error.
  >  Can you post a video about it?
  
  
  Ah, sorry. Didn't realize there were more steps required to trigger the 
problem:
  
  - Set mouse behaviour to "double-click".
  - Double-click on file to trigger "Overwrite" warning, press Cancel on the 
warning dialog.
  - Try to enter a directory by double-clicking. This will also result in a 
"Overwrite" warning, while it shouldn't.
  
  ---
  
  > Selecting a file by
  
  Sorry that I changed this in the summary, but I find it very confusing to 
talk about "select" here, because it can have so many different meanings…

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg, ngraham, rkflx
Cc: ltoscano, rkflx, broulik, jtamate, ngraham, #frameworks, michaelh, bruns

D12538: Allow accepting by double-click in save dialog

[Henrik Fehlauer]

rkflx retitled this revision from "Always prompt for overwrite on double-click 
in save dialog" to "Allow accepting by double-click in save dialog".
rkflx edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg, ngraham, rkflx
Cc: ltoscano, rkflx, broulik, jtamate, ngraham, #frameworks, michaelh, bruns

D12591: KFileWidget: Provide faster access to the icon position setting

[Henrik Fehlauer]

rkflx added a dependent revision: D12598: FileView: Provide faster access to 
the icon position setting.

REPOSITORY
  R241 KIO

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

To: rkflx, #frameworks
Cc: michaelh, bruns

D12592: KFileWidget: Prevent places panel width from growing 1px iteratively

[Henrik Fehlauer]

rkflx added a comment.


  Sorry for the wait, writing the commit message for this one and the alignment 
patch meant going through a //lot// of notes. Who'd knew that after the 
off-by-one error in Spectacle which only happened in a VM there could be even 
more difficult problems :D

REPOSITORY
  R241 KIO

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

To: rkflx, #frameworks
Cc: michaelh, bruns

D12591: KFileWidget: Provide faster access to the icon position setting

[Henrik Fehlauer]

rkflx added a comment.


  If this gets approved, I'll do the same change to K3b for consistency, which 
was the only other app showing the submenu I could find on https://lxr.kde.org.

REPOSITORY
  R241 KIO

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

To: rkflx, #frameworks
Cc: michaelh, bruns

D12592: KFileWidget: Prevent places panel width from growing 1px iteratively

[Henrik Fehlauer]

rkflx added a dependent revision: D12594: KFileWidget: Perfectly align filename 
widget with icon view.

REPOSITORY
  R241 KIO

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

To: rkflx, #frameworks
Cc: michaelh, bruns

D12594: KFileWidget: Perfectly align filename widget with icon view

[Henrik Fehlauer]

rkflx created this revision.
rkflx added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.
rkflx requested review of this revision.

REVISION SUMMARY
  Since the redesign of the file dialog for KDE4, the left edges of the
  line edits below the icon view were supposed to be aligned to the left
  edge of the icon view, while in KDE3 the naming labels were aligned
  instead.
  
  However, in practice that never worked out perfectly, there was always a
  small displacement. While hardly noticable in KDE4's widget styles, in
  particular for Breeze in KF5 with its small splitter handle width and
  larger layout spacing this became much more annoying to look at.
  
  This can be fixed by taking the splitter handle width as well as the
  grid layout spacing into account. While the patch seems straightforward,
  making the patch work with all styles was actually suprisingly
  difficult, not only because of the bug in the dependent patch.
  
  Depends on D12592 
  
  Ref T8552 
  
  FIXED-IN: 5.46

TEST PLAN
  `kdialog --getopenfilename`, check alignment with KMag for
  different widget styles (e.g. Breeze, Oxygen, Windows and Fusion) after
  startup, and after moving the splitter, resizing the dialog and toggling
  the places panel.
  
  Before:
  F5826739: KIO-alignment-before.png 
  
  After:
  F5826740: KIO-alignment-after.png 

REPOSITORY
  R241 KIO

BRANCH
  fix-alignment (branched from master)

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: rkflx, #frameworks
Cc: michaelh, bruns

D12592: KFileWidget: Prevent places panel width from growing 1px iteratively

[Henrik Fehlauer]

rkflx added a dependent revision: D12593: KFileWidget: Save places panel width 
also after hiding panel.

REPOSITORY
  R241 KIO

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

To: rkflx, #frameworks
Cc: michaelh, bruns

D12593: KFileWidget: Save places panel width also after hiding panel

[Henrik Fehlauer]

rkflx created this revision.
rkflx added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.
rkflx requested review of this revision.

REVISION SUMMARY
  In certain situations the user-set width of the places panel was not
  remembered.
  
  As `placesViewWidth` is initialized by `configGroup.readEntry` and
  updated in reaction to user actions, it makes sense to also save the
  same variable back to the config file, instead of looking directly at
  the `placesViewSplitter`.
  
  Depends on D12592 
  
  Ref T8552 
  
  FIXED-IN: 5.46

TEST PLAN
  `kdialog --getopenfilename`, change width of places panel, hide places
  panel with [F9], close dialog by clicking on Cancel, reopen
  dialog, close dialog a second time, reopen dialog, press [F9] again
  and check that previously set width is restored.

REPOSITORY
  R241 KIO

BRANCH
  fix-hidden-places-width-saving (branched from master)

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: rkflx, #frameworks
Cc: michaelh, bruns

D12592: KFileWidget: Prevent places panel width from growing 1px iteratively

[Henrik Fehlauer]

rkflx created this revision.
rkflx added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.
rkflx requested review of this revision.

REVISION SUMMARY
  cc00bbb22010 
 
tried to prevent the places panel from reopening with a
  width reduced by 1px compared to the previous invocation of the file
  dialog.
  
  While the patch worked on the surface when it was applied, the actual
  reasoning was wrong: The difference was not because of "extra resize()
  events", but because of a mismatch between what was read from the config
  (`placesViewWidth`) and what was written to the config (`sizes[0]`).
  
  The actual reason `placesViewWidth` and `sizes[0]` are different is
  because of a previous error: It is assumed that the splitter sizes sum
  up to `availableWidth`, while in reality this width also contains the
  width of the splitter handle. Upon `setSizes`, Qt equally takes away
  width from both sides of the splitter to make everything fit.
  
  While for the Oxygen and Fusion styles (3px and 4px splitter handle
  width, respectively) an adjustment of 1px did the trick, for Breeze with
  its splitter handle width of only 1px this did not work anymore due to
  rounding errors, it resulted in the config entry incrementing by 1px for
  every invocation of the dialog.
  
  To fix the problem for every style, we simply take the width of the
  splitter handle into account and revert the previous fix. While we are
  at it, we refactor to make everything more DRY.
  
  Ref T8552 
  
  FIXED-IN: 5.46

TEST PLAN
  Setup monitoring with `watch grep Speedbar ~/.config/kdeglobals`.
  Repeatedly open `kdialog --getopenfilename`, click on Cancel and
  check that the config entry does not change. Test for multiple widget
  styles (e.g. Breeze, Oxygen, Windows and Fusion), as well as for hiding
  the places panel and manually moving the splitter to both ends.

REPOSITORY
  R241 KIO

BRANCH
  fix-places-width-growth (branched from master)

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: rkflx, #frameworks
Cc: michaelh, bruns

D12591: KFileWidget: Provide faster access to the icon position setting

[Henrik Fehlauer]

rkflx created this revision.
rkflx added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.
rkflx requested review of this revision.

REVISION SUMMARY
  The file dialog allows to move the file name labels from next to the
  icon to right under the icon for Short View. However, having to go in
  a deeply nested Settings button > Icon Position > Above File Name
  submenu makes it both hard to discover and cumbersome to use. Ideally
  this setting could be accessed directly from the menu for good usability.
  
  Making it easy to change back settings is especially important in light
  of D12326 , which will enable Icons Above 
File Name by default.
  
  We linearize the submenu entries from the submenu to the top-level menu,
  which works just fine as there are only two entries, increasing the
  overall menu size by only one item while removing an entire submenu.
  
  While the submenu itself is not advertised in the API docs, it was
  publicly accessible, and therefore is kept for compatiblity reasons
  (although with a slight change in wording).
  
  Ref T8552 
  
  FIXED-IN: 5.47

TEST PLAN
  `kdialog --getopenfilename`, switch view modes via the settings
  button, check that the icon position settings are only enabled for
  View > Short View and still work properly.
  
  Before:
  F5826732: KIO-icon-position-before.png 
  
  After:
  F5826733: KIO-icon-position-after.png 

REPOSITORY
  R241 KIO

BRANCH
  icon-position-usability (branched from master)

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

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp
  src/filewidgets/kfilewidget.cpp

To: rkflx, #frameworks
Cc: michaelh, bruns

D12590: KFileWidget: Disable zoom buttons once reached minimum or maximum

[Henrik Fehlauer]

rkflx created this revision.
rkflx added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.
rkflx requested review of this revision.

REVISION SUMMARY
  In Gwenview the zoom buttons will be disabled when clicking on it would
  not result in any further change of zoom level, e.g. Zoom In is
  disabled for the largest icon size. In general it is recommended to
  disable unavailable actions.
  
  This patch adds the functionality to the file dialog too.
  
  Ref T8552 
  
  FIXED-IN: 5.46

TEST PLAN
  `kdialog --getopenfilename`, change icon size to minimum and maximum by
  sliding, mouse wheeling, pressing buttons and using the keyboard, and
  check that buttons are disabling and enabling properly.
  
  F5826731: KIO-disable-zoom-buttons.webm 

REPOSITORY
  R241 KIO

BRANCH
  disable-zoom-buttons (branched from master)

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: rkflx, #frameworks
Cc: michaelh, bruns

D12588: KFileWidget: Set minimum size for zoom slider

[Henrik Fehlauer]

rkflx created this revision.
rkflx added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.
rkflx requested review of this revision.

REVISION SUMMARY
  When choosing a very narrow width for the file dialog, the zoom slider
  for the icon size would get squished so that only the handle would be
  visible and the slider could not be used anymore to change zoom levels
  in any way.
  
  This can be avoided by setting a minimum size. 40px are smaller than the
  default size, but still allow to operate the slider comfortably.
  
  The minimal width of the dialog is unchanged, as before the toolbar will
  show the elision button once elements start overflowing.
  
  Ref T8552 
  
  FIXED-IN: 5.46

TEST PLAN
  `kdialog --getopenfilename`, change window size and observe zoom slider.
  
  F5826728: KIO-slider-min-size.webm 

REPOSITORY
  R241 KIO

BRANCH
  slider-min-size (branched from master)

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: rkflx, #frameworks
Cc: michaelh, bruns

D12538: Always prompt for overwrite on double-click in save dialog

[Henrik Fehlauer]

rkflx added a comment.


  TBH, I don't get what the overwrite and release schedule discussion has to do 
with this patch. All it does is add an additional method to trigger `slotOk()`, 
i.e. instead of clicking on Save, users can simply double-click.
  
  The fact that either the dialog or the app ask before overwriting in response 
to the slot is not changed in any way here. Don't judge a Diff by its title, in 
particular as long as the request to "Please update your summary too" is still 
in state "red" ;)
  
  ---
  
  Regarding what you were discussing: If I understood Peter's analysis in 
D12346  correctly, the API will always 
allow apps to opt-out of the dialog providing the overwrite confirmation, and 
instead add their own. Therefore to me it seems like it is only a matter of 
going through all apps and opt-in to the already existing functionality, i.e. I 
don't see why this would require changes in KIO which would result in 
dependency issues (unless I'm mistaken?).

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg, ngraham, rkflx
Cc: ltoscano, rkflx, broulik, jtamate, ngraham, #frameworks, michaelh, bruns

D12538: Select item without clicking the Open/Save button

[Henrik Fehlauer]

rkflx added a comment.


  > [remove] BUG: 267749
  
  Now I'm confused: Isn't what we do here exactly what the bug reporter wanted?

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg, ngraham, rkflx
Cc: rkflx, broulik, jtamate, ngraham, #frameworks, michaelh, bruns

D12538: Select item without clicking the Open/Save button

[Henrik Fehlauer]

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


  Thanks, works great now for single click users. Glad we could skip a huge 
discussion ;)
  
  However, for double click mode and Save, descending into directories is kinda 
broken. You might want to fix that before shipping…
  
  > Activate files in the filepicker without having to click on the Open/Save 
button first, but instead single/double clicking on the icon, depending on the 
mouse settings.
  
  Please update your summary too, and ideally reference those 3 commits I 
mentioned, as they contain useful information and you copied most of the patch 
from there.
  
  In D12538#254372 , @ngraham wrote:
  
  > (As mentioned before, this can't and won't land until other things have 
been fixed and implemented first)
  
  
  @ngraham Is this still the case with the changed scope of the patch?

INLINE COMMENTS

> kfilewidget.cpp:1179
> +// accept). This way the user can choose a file and add a "_2" for 
> instance to the filename.
> +// Double clicking however will trigger this, regardless of 
> single/double click mouse setting,
> +// see: _k_slotViewDoubleClicked

Do you mean "override" instead of "trigger" here?

> kfilewidget.cpp:2156-2162
> +void KFileWidgetPrivate::_k_slotViewDoubleClicked(const QModelIndex )
> +{
> +if (operationMode == KFileWidget::Saving && index.isValid()) {
> +q->slotOk();
> +}
> +}
> +

I'd move that right under `:_k_slotIconSizeSliderMoved`, to keep ordering at 
least somewhat consistent with the declaration.

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg, ngraham, rkflx
Cc: rkflx, broulik, jtamate, ngraham, #frameworks, michaelh, bruns

D12544: Don't select file extension

[Henrik Fehlauer]

rkflx accepted this revision.
rkflx added a comment.


  Thanks for the patch. Pondered over the code, but could not find anything 
wrong.
  
  In D12544#254554 , @anemeth wrote:
  
  > I removed these checks: `&& !locationEdit->isVisible()`
  >  I assume at one point the filename line edit was once set to hide/show on 
demand.
  
  
  Beineri originally implemented this in 0134fb3a5e50 
, 
apparently. However, I don't understand why the check has a `!`. How could this 
ever work? At least in the latest KDE3 it is already broken.

REPOSITORY
  R241 KIO

BRANCH
  select_filename_only (branched from master)

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

To: anemeth, #frameworks, #vdg, ngraham, rkflx
Cc: rkflx, ngraham, #frameworks, michaelh, bruns

D12545: Set focus on the filename line edit when a file is selected

[Henrik Fehlauer]

rkflx edited the summary of this revision.
rkflx edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg, ngraham, rkflx
Cc: rkflx, ngraham, #frameworks, michaelh, bruns

D12538: Select item without clicking the Open/Save button

[Henrik Fehlauer]

rkflx added a comment.


  > https://bugs.kde.org/show_bug.cgi?id=267749
  
  Wow, reading the bug again, my suggestions is fitting right in! Double click 
to trigger Save for single click users. Only after some confusion in Comment 1 
the only slightly related question of overwriting came up in Comment 4, but 
this is not what the bug was originally about…

REPOSITORY
  R241 KIO

BRANCH
  doubleclick_save (branched from master)

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

To: anemeth, #frameworks, #vdg, ngraham
Cc: rkflx, broulik, jtamate, ngraham, #frameworks, michaelh, bruns

D12538: Select item without clicking the Open/Save button

[Henrik Fehlauer]

rkflx added a comment.


  After sleeping over this, I don't feel comfortable the way this is supposed 
to work for single click: Selection markers only really make sense when 
enabling multi-select mode. Abusing a single selection marker in single-select 
mode to mean "put the filename in the line edit so it is editable" feels really 
weird, and if you are doing this more often than overwriting (which might be 
very likely!) hitting the small icon is quite cumbersome.
  
  IOW, I don't think the workaround which was promised to @jtamate's concern is 
good enough, and suggesting to switch to double click only to sustain a "pure" 
single click does not make sense to me either.
  
  The question for single click is: What is the primary action which should be 
triggered immediately? Is it overwriting, or is it putting the filename in the 
field? For Dolphin and the Open dialog it is clear that actually opening is the 
primary action. For Save I'd argue that overwriting is only the secondary 
option, and thus should be triggered by the second click only.
  
  I did some digging, and indeed this seems to be how it was originally 
implemented: 4ccd77256a0c 
 
added what this patch is now trying to remove again. Then right after that, 
f729c291cdbb 
 
allowed double clicking to overwrite without moving the mouse to the Save 
button. Somehow the latter commit got broken later on, I guess in 7f08f13809bc 
 
("Hope I didn't miss anything." haha). Maybe that slot should have triggered 
only for `KFileWidget::Saving`?
  
  Given the concerns with selection markers in single-select mode and thus for 
Save in general, and them being potentially far off in the future, can we think 
about changing this patch to double-click to overwrite, so single-click would 
still allow to append `_2` to the filename, a likely more common operation? 
Users who set click behaviour to double click everywhere would not be affected 
by this change at all!
  
  ---
  
  Historical side note: In KDE3 with single click enabled, the first click 
would select in file dialogs, the second click would accept. In KDE4 accepting 
was refined to single click for Open, but the double click was kept for Save, 
which makes sense to me.
  
  (And please think twice before making this discussion about a general single 
vs. double click debate again; I'm trying to improve single click to make more 
sense for the common use cases here, as we agreed upon as a sensible direction. 
Thanks ;)

REPOSITORY
  R241 KIO

BRANCH
  doubleclick_save (branched from master)

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

To: anemeth, #frameworks, #vdg, ngraham
Cc: rkflx, broulik, jtamate, ngraham, #frameworks, michaelh, bruns

D12328: Enable preview by default in the filepicker dialog

[Henrik Fehlauer]

rkflx accepted this revision.
rkflx added a subscriber: elvisangelaccio.
rkflx added a comment.
This revision is now accepted and ready to land.


  LGTM now (even though you ignored the "newline" I suggested…)
  
  I'd appreciate a second opinion from #Frameworks 
 on the new `kconf_update` 
directory, though.
  
  Also, the very idea of enabling previews by default in the file picker (as 
was done in Dolphin for 18.04) should get some more buy-in. I feel bad for 
tagging @elvisangelaccio again, but maybe you have suggestions for someone else 
who might help out in reviewing some of the patches for the file dialog?

BRANCH
  preview_default (branched from master)

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

To: anemeth, #frameworks, #vdg, rkflx
Cc: elvisangelaccio, abetts, rkflx, ngraham, #frameworks, michaelh, bruns

D12545: Set focus on the filename line edit when a file is selected

[Henrik Fehlauer]

rkflx added a comment.


  In D12545#254479 , @ngraham wrote:
  
  > No, it doesn't actually work. Turn on double-click,
  
  
  Aha! This is what I'm missing. It should be mentioned in the summary if 
non-default settings are used :D

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg, ngraham, rkflx
Cc: rkflx, ngraham, #frameworks, michaelh, bruns

D12545: Set focus on the filename line edit when a file is selected

[Henrik Fehlauer]

rkflx added a comment.


  In D12545#254475 , @ngraham wrote:
  
  > Good point @rkflx, we definitely don't want to break keyboard navigation. 
We want this behavior only when clicking, and only for the save dialog.
  
  
  Yeah, but not sure I understand. Isn't this exactly the behaviour we 
currently have, without the patch?

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg, ngraham, rkflx
Cc: rkflx, ngraham, #frameworks, michaelh, bruns

D12545: Set focus on the filename line edit when a file is selected

[Henrik Fehlauer]

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


  Hm, this breaks selecting files and even navigating directories with the 
keyboard (e.g. via the arrow keys), and as such cannot possibly be something we 
want.
  
  I could imagine a different spin here: [⇥] should switch focus from the item 
view to the name line edit, which it currently does not. (And as the dialog 
starts with focus on the name line edit, [⇧] + [⇥] should switch focus 
immediately to the item view.)
  
  Probably needs proper tabstop ordering.
  
  ---
  
  In D12545#254437 , @ngraham wrote:
  
  > This is clearly what the code was //trying// to do, based on inline 
comments: 
https://cgit.kde.org/kio.git/tree/src/filewidgets/kfilewidget.cpp#n1176
  
  
  From observing the behaviour, to me this seems to affect the save dialog, and 
only upon //clicking// on a file. When navigating via the keyboard and thus 
merely //selecting// a file, this does not seem to be called. IOW, this part of 
the code seems fine to me.

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg, ngraham, rkflx
Cc: rkflx, ngraham, #frameworks, michaelh, bruns

D12328: Enable preview by default in the filepicker dialog

[Henrik Fehlauer]

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


  Thanks, `arc patch` now works great for me, automatically applying the 
dependent revision as it should.
  
  In D12328#254098 , @anemeth wrote:
  
  > In the kconf_update log file 
`/home/alex/.kde4/share/apps/kconf_update/log/update.log` I found the following 
entry:
  
  
  `.kde4` likely won't contain anything relevant for a KF5 KIO ;)
  
  I could not find an equivalent file in `~/.config`, but got some warnings 
here:
  
journalctl /usr/lib64/libexec/kf5/kconf_update --follow
  
  > Is there something wrong with the .upd file?
  
  See inline comment on what I had to fix to make it work…

INLINE COMMENTS

> CMakeLists.txt:2
> +
> +install(FILES filepicker-remove-previews-conf.upd
> +DESTINATION ${KDE_INSTALL_KCONFUPDATEDIR})

I'd name this file just `filepicker.upd`, as this way more updates could be 
added later on, and it would be more consistent with the naming of the other 
`upd` files.

> filepicker-remove-previews-conf.upd:1
> +Id=kdeglobals-filepicker-remove-previews-conf
> +File=kdeglobals

Most of the update files seem to have `Version=5` in the first line, and a 
newline after that.

And indeed, this is exactly the reason why it did not work:

  kconf_update[5695]: "Missing \"Version=5\", file 
'share/kconf_update/filepicker-remove-previews-conf.upd' will be skipped."

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

To: anemeth, #frameworks, #vdg, rkflx
Cc: abetts, rkflx, ngraham, #frameworks, michaelh, bruns

D12385: Thumbnail smooth scaling in filepicker

[Henrik Fehlauer]

rkflx accepted this revision.
rkflx added a comment.


  Thanks Kai, great questions. I think I can answer them:
  
  In D12385#250511 , @broulik wrote:
  
  > What's the performance penalty of that?
  
  
  I don't see any performance issues, both when testing the perceived 
performance, as well as when looking at the code.
  
  > Does the file dialog request the correct size from the preview job?
  
  It's always scaling down 128px icons (i.e. the "normal" thumbnail size) and 
never touches 256px icons ("large"), because 128px is the maximum setting of 
the slider (icon sizes verified with `qDebug`). (For HiDPI, thumbnail handling 
is broken like everywhere, sadly.)
  
  > It shouldn't have to end up in this codepath that often, I //think//.
  
  This code path is called once per item upon entering directories, switching 
view modes or changing zoom levels, as well as when hovering over an item.
  
  ---
  
  I'd say the patch is okay. We get better visuals with no perceived slow-down 
(maybe a theoretical performance loss, but not really relevant for those tiny 
thumbnails, apparently).
  
  As for the intermittent issues described above: Maybe recreating the cache 
due to 371e523f5d7e 
 is 
the reason while comparing patched version and system version of the file 
dialog?

REPOSITORY
  R241 KIO

BRANCH
  image_smooth_downscale (branched from master)

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

To: anemeth, #frameworks, #vdg, ngraham, elvisangelaccio, rkflx
Cc: cfeck, broulik, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx accepted this revision.
rkflx added a subscriber: elvisangelaccio.
rkflx added a comment.


  Cool, LGTM now. Due to the string change this will have to wait for 5.47, and 
we have to find a general consensus in #frameworks 
 about the nature of the patch 
first, of course.
  
  @elvisangelaccio We'd love to hear your opinion on the feature and on the 
code, and we'd be interested whether the same change would be a good idea for 
#Dolphin  ;)

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12328: Enable preview by default in the filepicker dialog

[Henrik Fehlauer]

rkflx added a comment.


  Thanks for the update, but please make sure not to lose the `kconf_update` 
changes…

INLINE COMMENTS

> kdiroperator.cpp:2163
> +d->showPreviews = configGroup.readEntry(QStringLiteral("Show Inline 
> Previews"), true);
> +d->showPreviewsConfigEntry = d->showPreviews;
>  }

Ah, here it is. This really belongs in the other patch.

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

To: anemeth, #frameworks, #vdg, rkflx
Cc: abetts, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  Thanks. Still not 100% there, though ;) (And please rebase on current master, 
while you are at it…)

INLINE COMMENTS

> kdiroperator.cpp:2622
>  } else {
> -const QFontMetrics metrics(itemView->viewport()->font());
> +const QFontMetrics metrics(itemView->viewport()->font());
>  int size = itemView->iconSize().height() + metrics.height() * 2;

You still got an unrelated whitespace change here (check on Phabricator with 
Revision Contents > History > Whitespace Changes > Show All).

> kdiroperator.cpp:2166
>  d->showPreviews = configGroup.readEntry(QStringLiteral("Previews"), 
> false);
> +d->showPreviewsEnabledBeforeZoom = d->showPreviews;
>  }

Don't remove that, otherwise your reviewer will be totally confused and wonder 
why your patch is suddenly totally broken! :D

(I suspect you already removed what Nate wanted in an earlier revision, but the 
rest should be kept, of course…)

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12306: Improve grid icon layout in filepicker dialog

[Henrik Fehlauer]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:bbc262674a56: Improve grid icon layout in filepicker 
dialog (authored by anemeth, committed by rkflx).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12306?vs=32680=33094

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

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp

To: anemeth, #frameworks, #vdg, ngraham, rkflx
Cc: abetts, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

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


  @anemeth Finally got to the code review part. First of all, thanks for 
implementing the disabled/turned-off behaviour I suggested, works really great 
now and surely makes the feature much more pleasant to use.
  
  Some small comments regarding the code (better naming of variables and 
cleanup, mostly), but otherwise looks pretty good.

INLINE COMMENTS

> kdiroperator.cpp:232
>  void updateListViewGrid();
> +void updateIconPreviewsState();
>  int iconSizeForViewType(QAbstractItemView *itemView) const;

I'd name this `updatePreviewActionState` to keep the terminology somewhat 
consistent (otherwise the code is a bit hard to follow, at least I felt this 
way when reviewing the patch).

> kdiroperator.cpp:286
>  bool showPreviews;
> +bool calledByPreviewStateUpdate;
> +bool showPreviewsEnabledBeforeZoom;

`calledFromUpdatePreviewActionState`

> kdiroperator.cpp:287
> +bool calledByPreviewStateUpdate;
> +bool showPreviewsEnabledBeforeZoom;
> +

`showPreviewsConfigEntry`

> kdiroperator.cpp:1646-1647
>  
> +// set grid and previews
> +d->updateListViewGrid();
> +

Unrelated?

> kdiroperator.cpp:2131
>  //}
> -if (configGroup.readEntry(QStringLiteral("Show Preview"), false)) {
> +if (configGroup.readEntry(QStringLiteral("Show Preview"), true)) {
>  d->defaultView |= KFile::PreviewContents;

That's a totally unrelated change (and a change which I would object to). `Show 
Preview` is about the preview sidebar you can toggle with [F11].

> kdiroperator.cpp:2585
>  } else {
> -const QFontMetrics metrics(itemView->viewport()->font());
>  int size = itemView->iconSize().height() + metrics.height() * 2;

Unrelated change.

> kdiroperator.cpp:2587
> +// hide icon previews when they are too small
> +const bool isIconSizeBigEnoughForPreview = itemView->iconSize().height() 
> > metrics.height() * 2;
> +

Please drop the `is` in the variable name, as this would only be used for the 
corresponding getter function normally.

> kdiroperator.cpp:2589-2591
> +if (showPreviews && isIconSizeBigEnoughForPreview) {
> +showPreviewsEnabledBeforeZoom = true;
> +}

Could you explain why you need to change `showPreviewsEnabledBeforeZoom` here? 
As far as I can see this variable just caches the config value, and thus should 
only be set in `_k_toggleInlinePreviews`?

Do you have an example where something would break if we would remove this code 
block?

> kdiroperator.cpp:2599
> +} else {
> +previewAction->setToolTip(i18n("Icon size is too small for 
> preview"));
> +}

Wording: "for preview" → "for showing previews"

(Or just use @ngraham's wording instead of inventing your own, because as a 
native speaker he often gets it quite right: "Previews are automatically 
disabled for icons smaller than {threshold]", or simply "Previews are 
automatically disabled for small icon sizes.".)

> kdiroperator.cpp:2615
> +
> +const QFontMetrics metrics(itemView->viewport()->font());
> +

Unrelated change.

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12328: Enable preview by default in the filepicker dialog

[Henrik Fehlauer]

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


  In D12328#252246 , @rkflx wrote:
  
  > In D12328#250028 , @anemeth 
wrote:
  >
  > > Rebase on D12321 
  >
  >
  > While the dependency tree is now looking good, the rebase did not work out 
correctly. While it might look correct on your local branch, on Phabricator you 
can clearly see that in this Diff the changes from the parent revision are 
visible, which should not be there. Please make sure to always check what ends 
up on Phabricator, as that's what your reviewers are seeing.
  >
  > `arc diff` will upload the commit range between `HEAD` and the upstream 
tracking branch. Here the upstream branch apparently is `master`, while it 
should be your local branch where D12321  
is living (you have a separate branch per Diff as written in the wiki, right?). 
You may want to look at my advice in D12321#249646 
 again.
  
  
  @anemeth Ping. I won't be able to help you with `kconf_update` or review the 
patch unless Phab shows me the proper patch!

INLINE COMMENTS

> kdiroperator.cpp:2165
>  if (d->inlinePreviewState == Private::NotForced) {
> -d->showPreviews = configGroup.readEntry(QStringLiteral("Previews"), 
> false);
> +d->showPreviews = configGroup.readEntry(QStringLiteral("Preview 
> Thumbnails"), true);
> +d->showPreviewsEnabledBeforeZoom = d->showPreviews;

There's still some confusion in the wording wrt. `Show Preview`.

How about naming your setting `Show Inline Previews` (inspired by 
`_k_toggleInlinePreview()`)?

(The other option could be renamed to `Show Aside Preview` in a followup patch.)

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg, rkflx
Cc: abetts, rkflx, ngraham, #frameworks, michaelh, bruns