D14360: Remove custom icon selection for trash

shubham updated this revision to Diff 38824. shubham added a comment. made above requested changes REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14360?vs=38637=38824 REVISION DETAIL https://phabricator.kde.org/D14360 AFFECTED FILES

D14449: Modify device usage information

ngraham added a comment. In D14449#300868 , @rkflx wrote: > 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?

D14501: Top-align labels in properties dialog

ngraham accepted this revision. ngraham added a comment. Ah, so much better. 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,

D14360: Remove custom icon selection for trash

cfeck added inline comments. INLINE COMMENTS > kfileplaceeditdialog.h:128 > + */ > +bool canEditIcon() const; > Why is this a public API function? If it needs to be public, it should have better documentation. There is no 'setIconEditable()' function, so a hint why some icons are

D14501: Top-align labels in properties dialog

cfeck accepted this revision. cfeck added a comment. This revision is now accepted and ready to land. Merci :) 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.

D14505: Sync set/send/update methods

davidedmundson created this revision. davidedmundson added a reviewer: KWin. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. davidedmundson requested review of this revision. REVISION SUMMARY Currently whenever a single client

D14504: Save few string allocations

apol created this revision. apol added reviewers: Frameworks, ilic. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. apol requested review of this revision. REVISION SUMMARY We are not using them on their own, so it's doable.

D14503: Android: also fall-back to using QLocale

apol created this revision. apol added reviewers: Frameworks, ilic, aacid. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. apol requested review of this revision. REVISION SUMMARY There's none of these environment variables

D14502: Reuse function that already does the same

apol created this revision. apol added reviewers: Frameworks, ilic. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. apol requested review of this revision. TEST PLAN Tests still pass REPOSITORY R249 KI18n BRANCH master

D14473: Fix KCatalog::translate when translation is same as original text

This revision was automatically updated to reflect the committed changes. Closed by commit R249:34a22e5ee5d4: Fix KCatalog::translate when translation is same as original text (authored by aacid). REPOSITORY R249 KI18n CHANGES SINCE LAST UPDATE

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added a comment. I think the following yields quite good results: qreal distance(int width, int height, int desiredWidth, int desiredHeight) { auto targetSamples = desiredWidth * desiredHeight; auto xscale = (1.0 * desiredWidth) / width; auto yscale =

D14360: Remove custom icon selection for trash

ngraham added inline comments. INLINE COMMENTS > kfileplaceeditdialog.h:126 > +/** > + * @returns check for icon's editabilty > + */ Change the text to "@returns whether the item's icon is editable" REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D14360 To:

D14360: Remove custom icon selection for trash

ngraham requested changes to this revision. ngraham added a comment. This revision now requires changes to proceed. This works and doesn't crash Gwenview for me anymore, yay! Please fix the below issues. And @pino, does the code look sensible now? INLINE COMMENTS >

D14501: Top-align labels in properties dialog

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

D14449: Modify device usage information

dhaumann added a comment. @rkflx Would be fine with this, as long as the alignment is correct (i.e. the text "Device capacity: ==- 24% used of 94.4 Gib" should all be one one line, and the text " 22.5 GiB used, 71.9 GiB free" should be below. The screenshot above is an

D14499: Use in-class member initialization where possible

dhaumann created this revision. dhaumann added a reviewer: vkrause. Restricted Application added projects: Kate, Frameworks. Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. dhaumann requested review of this revision. REVISION SUMMARY Allows to default some

D14290: [KWidgetJobTracker] Show "Open Destination" etc buttons only if destination is valid

dfaure added a comment. Why is this called with an invalid URL? The bug report says "copy/move a big file to another device/partition." -- surely the destination should be valid? It sounds like this patch is masking a real bug... REPOSITORY R288 KJobWidgets REVISION DETAIL

D14290: [KWidgetJobTracker] Show "Open Destination" etc buttons only if destination is valid

dfaure requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R288 KJobWidgets REVISION DETAIL https://phabricator.kde.org/D14290 To: broulik, dfaure Cc: elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

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

D14449: Modify device usage information

shubham added a comment. no problem 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: Modify device usage information

dhaumann added a comment. Correct, but "visual cleanness" goes over redundancy (imo). In fact, the information 100 GiB, 20 GiB used (20%), 80 GiB free is also highly redundant. So redundancy is not the argument here. Instead, I think we should follow the best visual design. And the

D14449: Modify device usage information

ngraham added a comment. In D14449#300842 , @shubham wrote: > dhaumann: but that deviec capacity label seems to be redundant, because capacity is simply used + free Yeah, but you have to do mental math to calculate that. It doesn't make

D14449: Modify device usage information

shubham added a comment. In D14449#300841 , @dhaumann wrote: > I think it looks visually distracting that one progress bar line ends up in two lines of text - this should be avoided. > > To behonest, the suggestion by @ngraham makes most

D14449: Modify device usage information

dhaumann added a comment. I think it looks visually distracting that one progress bar line ends up in two lines of text - this should be avoided. To behonest, the suggestion by @ngraham makes most sense to me: Two lines with the distinction of "Device capacity" and "Device usage".

D14434: add functions to access keywords

This revision was automatically updated to reflect the committed changes. Closed by commit R216:9e3f07de97ea: add functions to access keywords (authored by dhaumann). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D14434?vs=38629=38799#toc REPOSITORY R216 Syntax Highlighting CHANGES

D14451: Add Definition::::formats()

This revision was automatically updated to reflect the committed changes. Closed by commit R216:273bfa2d75ba: Add Definitionformats() (authored by dhaumann). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D14451?vs=38721=38798#toc REPOSITORY R216 Syntax Highlighting CHANGES SINCE

D14468: Add QVector Definition::includedDefinitions() const

This revision was automatically updated to reflect the committed changes. Closed by commit R216:3d3b4589e9e2: Add QVectorDefinition Definition::includedDefinitions() const (authored by dhaumann). REPOSITORY R216 Syntax Highlighting CHANGES SINCE LAST UPDATE

D14471: Add Theme::TextStyle Format::textStyle() const;

This revision was automatically updated to reflect the committed changes. Closed by commit R216:0225592f54cf: Add Theme::TextStyle Format::textStyle() const; (authored by dhaumann). REPOSITORY R216 Syntax Highlighting CHANGES SINCE LAST UPDATE

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added inline comments. INLINE COMMENTS > anthonyfieroni wrote in icoutils_common.cpp:33 > Comparing doubles should be done by epsilon not by <= 0.0 Not in general. There is nothing inexact here. Also, if difference ~= 0, it does not matter if you scale by 1, 2, -1 or -2. REPOSITORY

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added a comment. Criteria for a good selection algorithm: 1. If there is an exact fit, use it 2. If all candidates have the same aspect ratio, use the best fit - prefer slight downscaling over slight upscaling - prefer slight upscaling over large downscaling 3. If aspect

D14308: [Exe Thumbnailer] Improve icon selection algorithm

anthonyfieroni added inline comments. INLINE COMMENTS > bruns wrote in icoutils_common.cpp:33 > @anthonyfieroni Why? Comparing doubles should be done by epsilon not by <= 0.0 REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D14308 To: broulik, #frameworks, dfaure,

D10040: Add serial number and EISA ID to OutputDevice interface

romangg added a comment. In D10040#300588 , @davidedmundson wrote: > I just removed handling of dynamically updating eisa/serialNumber it doesn't seem to be something that makes sense for it to change at runtime. > > Also I don't want to

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added inline comments. INLINE COMMENTS > anthonyfieroni wrote in icoutils_common.cpp:33 > It should be qFuzzyIsNull no? @anthonyfieroni Why? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D14308 To: broulik, #frameworks, dfaure, ngraham, pali, vonreth,

D14308: [Exe Thumbnailer] Improve icon selection algorithm

anthonyfieroni added inline comments. INLINE COMMENTS > icoutils_common.cpp:33 > +qreal delta = width - desiredWidth; > +delta = (delta >= 0.0 ? delta : -delta * 2 ); // Penalize for scaling up > + It should be qFuzzyIsNull no? REPOSITORY R320 KIO Extras REVISION DETAIL

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added inline comments. INLINE COMMENTS > broulik wrote in icoutils_common.cpp:33 > I'm open to suggestions … also technically you cannot assume the icon is > square Where do you see any assumption about square icons? REPOSITORY R320 KIO Extras REVISION DETAIL

D14449: Modify device usage information

shubham updated this revision to Diff 38784. shubham added a comment. Remove unintentional " "(that was silly) REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14449?vs=38740=38784 REVISION DETAIL https://phabricator.kde.org/D14449 AFFECTED FILES

D14449: Modify device usage information

ngraham added inline comments. INLINE COMMENTS > kpropertiesdialog.cpp:1270 > const int percentUsed = qRound(100.0 * qreal(used) / qreal(size)); > - > + > d->m_capacityBar->setText( Unnecessary/unintentional whitespace change. REPOSITORY R241 KIO REVISION DETAIL

D14308: [Exe Thumbnailer] Improve icon selection algorithm

broulik added inline comments. INLINE COMMENTS > bruns wrote in icoutils_common.cpp:33 > I think the formula is a little bit to ad-hoc, and actually wrong. > > We have 2 different notable cases. `desiredAspectRatio` obviously is a > constant, this leaves us with `candidateAspectRatio` and

D10040: Add serial number and EISA ID to OutputDevice interface

davidedmundson added a comment. I just removed handling of dynamically updating eisa/serialNumber it doesn't seem to be something that makes sense for it to change at runtime. Also I don't want to copy the current setEdid pattern, which is broken ATM. Whenever any new client connects it

D10040: Add serial number and EISA ID to OutputDevice interface

davidedmundson updated this revision to Diff 38772. davidedmundson added a comment. Remove unrelated changes REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10040?vs=38734=38772 BRANCH output_changes REVISION DETAIL https://phabricator.kde.org/D10040

D12388: Output device color curves correction

romangg accepted this revision. romangg added inline comments. INLINE COMMENTS > outputconfiguration.h:203 > /** > +<<< HEAD > * Scale rendering of this output. rm REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D12388 To: davidedmundson, #frameworks,

D10040: Add serial number and EISA ID to OutputDevice interface

romangg added inline comments. INLINE COMMENTS > outputdevice.cpp:161 > + int32_t subPixel, const char *make, const char > *model, > + int32_t transform) > { Unrelated change. > outputdevice_interface.cpp:153 > +connect(this,

D12709: Allow skipping the build of the KPackage install handlers when building `frameworkintegration`

apol added a comment. If you are building a flatpak, please use org.kde.Sdk. REPOSITORY R252 Framework Integration REVISION DETAIL https://phabricator.kde.org/D12709 To: tundracomp Cc: apol, kde-frameworks-devel, michaelh, ngraham, bruns

D14131: Add enum alias Property::Language for typo Property::Langauge

kossebau added a comment. @mgallien Enjoy your vacations :) Not a pressing issue, just some TODO which could be fixed already now and which I saw while stumpling about the same typo elsewhere. So fine with me to only handle once you are back. REPOSITORY R286 KFileMetaData REVISION DETAIL

Re: Pre-review CI

Am Montag, 30. Juli 2018, 06:50:47 CEST schrieb Bhushan Shah: > On Sun, Jul 29, 2018 at 05:00:19PM +0200, Friedrich W. H. Kossebau wrote: > > 1 job means one huge build log to look at, or? In that case I would prefer > > separate jobs. Given review requests are prone to fail. > > Thing is, you

D14397: Support libcanberra for audio notification

rjvbb added a comment. > I don't really care about your artificially created problems. You *have* libcanberra but want to use it for one project and not the other. Sorry, but no. Oh, and do you have to show ignorance? This is a very common problem in software packaging, part of what

D14397: Support libcanberra for audio notification

rjvbb added a comment. > (Oh btw it's not like I didn't try, I originally ported all of this to QtMultimedia but `QAudioEffect` which is for low-latency sound effects only supports WAV sounds and `QMediaPlayer` had significant overhead and initialization times as well which is why I

D14397: Support libcanberra for audio notification

broulik abandoned this revision. broulik added a comment. > I haven't checked, but I'd appreciate if that could also be done through a cmake option. In packaging systems like MacPorts and HB it's perfectly possible to have libcanberra and/or pulseaudio installed for Gnome apps that cannot do

D14473: Fix KCatalog::translate when translation is same as original text

ilic accepted this revision. ilic added a comment. This revision is now accepted and ready to land. I distinctly remember having had the same quandry once, and then someone from the gettext side said to compare pointers. Even the comment to dngettext call in the plural version of

D14435: Fix KTimeComboBox input mask for AM/PM times

glennw updated this revision to Diff 38748. glennw added a comment. Updated to include test for 24hr locales. REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14435?vs=38685=38748 REVISION DETAIL https://phabricator.kde.org/D14435 AFFECTED FILES

D14451: Add Definition::::formats()

vkrause accepted this revision. This revision is now accepted and ready to land. REPOSITORY R216 Syntax Highlighting BRANCH addFormatGetters (branched from master) REVISION DETAIL https://phabricator.kde.org/D14451 To: dhaumann, cullmann, vkrause Cc: kwrite-devel, kde-frameworks-devel,

D14451: Add Definition::::formats()

cullmann added a comment. Together with includedDefinitions() from the other request, this should be sufficient to fake the internal attribute vectors in ktexteditor. REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D14451 To: dhaumann, cullmann, vkrause

D14468: Add QVector Definition::includedDefinitions() const

vkrause accepted this revision. This revision is now accepted and ready to land. REPOSITORY R216 Syntax Highlighting BRANCH addIncludedDefinitions (branched from master) REVISION DETAIL https://phabricator.kde.org/D14468 To: dhaumann, vkrause, cullmann Cc: kwrite-devel,

D14471: Add Theme::TextStyle Format::textStyle() const;

vkrause accepted this revision. This revision is now accepted and ready to land. REPOSITORY R216 Syntax Highlighting BRANCH extendFormat (branched from master) REVISION DETAIL https://phabricator.kde.org/D14471 To: dhaumann, cullmann, vkrause Cc: kwrite-devel, kde-frameworks-devel,

D14449: Modify device usage information

shubham updated this revision to Diff 38740. shubham retitled this revision from "Display used space in GiB also" to "Modify device usage information". shubham added a comment. F6162515: Screenshot_20180730_113005.png Looks neat and tidy REPOSITORY

D14435: Fix KTimeComboBox input mask for AM/PM times

mlaurent requested changes to this revision. mlaurent added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > ktimecomboboxtest.cpp:198 > +QString mask = m_combo->lineEdit()->inputMask(); > +QCOMPARE(mask.contains(QLatin1String("aa")), true); > +delete