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

[Volker Krause]

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, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D14468: Add QVector Definition::includedDefinitions() const

[Volker Krause]

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, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D14451: Add Definition::::formats()

[Christoph Cullmann]

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
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D14451: Add Definition::::formats()

[Volker Krause]

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, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D14435: Fix KTimeComboBox input mask for AM/PM times

[Glenn Watson]

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&id=38748

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

AFFECTED FILES
  autotests/ktimecomboboxtest.cpp
  autotests/ktimecomboboxtest.h
  src/ktimecombobox.cpp

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

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

[Chusslove Illich]

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 KCatalog::translate mentions comparison of pointers to 
this effect. So, "someone" simply screwed up on porting from kdelibs4, when 
replacing char*'s with QByteArray's...

REPOSITORY
  R249 KI18n

BRANCH
  master

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

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

D14397: Support libcanberra for audio notification

[Kai Uwe Broulik]

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 
without them, but still not want to let other software use them (opportunistic 
dependencies)
  
  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. How about I cannot do without them like your Gnome apps? Then what. (Can't 
you override those variables externally using `-D...` anyway?)
  
  But whatever, I'm out. Was an attempt to fixup the subpar experience we had 
with notification sounds and I'll happily continue to use use this patch 
locally but I'm tired of this.
  
  (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 eventually ended up using canberra)

REPOSITORY
  R289 KNotifications

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

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[René J . V . Bertin]

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 eventually ended up using canberra)
  
  See, it would have been constructive to mention that earlier.
  
  FWIW, I think we just want to play a file here, not juggle with audio effects.
  
https://stackoverflow.com/questions/41197576/how-to-play-mp3-file-using-qaudiooutput-and-qaudiodecoder.
  
  Anyone else interested in the idea of having a QtMultiMedia Phonon backend 
that (a priori) uses the most low-level APIs from that Qt component?

REPOSITORY
  R289 KNotifications

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

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[René J . V . Bertin]

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 allows your distribution to warn you something 
you're trying to uninstall is a dependency for something you might want to keep 
installed.
  
  As I said, yes, there is a CMake mechanism to disable specific `find_package` 
calls but it's rather confidential, ugly and useless with custom findfoo.cmake 
implementations that bypass find_package.

REPOSITORY
  R289 KNotifications

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

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

Re: Pre-review CI

[Friedrich W. H. Kossebau]

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 don't care about the pre-review CI jobs as long as they
> are passing, but in case of fail, yes you might have to look at long
> log depending on where failure was, in first platform or last one.

So this is a use-case which should be optimized for, given it is an expected 
to hit hot-path of developer workflow :)

> > compare to non-review build jobs, I would assume. And having jobs separate
> > also means one gets results for any platforms, does not stop on the first
> > failing?
> 
> Yes, but it also means that if there is obvious mistake in review, all
> will fail, insteaad of bailing out earlier.

More advantages with parallel builds:
* bigger projects, where building takes some time (krita, kdevelop, plasma-*, 
etc) and where sometimes patches are reviewed almost in sync, parallel builds 
mean quicker feedback
* when doing fix-up for patches which fail on a platform, getting quicker 
feedback would be also good
* parallel builds might also mean easier report for results on the different 
platforms in the jenkins page, so one can quickly see on which platform there 
are issues (and which experts might be needed)
* a platform being broken on normal CI for some unrelated reason (outdated 
deps, platform issues) will also mean the review build breaks there, in that 
case any later platforms in the single job build would not be reached for 
review feedback
* platform-specific issues (like missing includes) which need platform experts 
will result in a fail before reaching other issues (like regressions in 
tests), having parallel builds would allow the non-build failing platforms to 
also run the tests and allow the developer to already investigate the test 
regression, while waiting for the platform experts to help out with the 
platform specific issues

So unless we are short of resources on CI, I really would favour separate 
builds, to always get feedback for all platforms, instead of doing some try & 
error walk through all platforms one by one, with the others having to wait 
for the earlier (and respective people to sort out things there).

Let's make sure things can be worked in parallel where possible, and as quick 
as possible, given most FLOSS contributors only have small time snippets to do 
things.

> > > - Should we send out comment for failure and success? Or is it easier to
> > > 
> > >   figure out the console log link without the comment? See linked review
> > >   for example [1].
> > 
> > [1] -> [2] here.
> > 
> > What do you mean exactly by "send out comment for failure and success"?
> > More emails? (Please not). That example works fine with me, but not sure
> > what the alternative is?
> 
> The alternative being, instead of jenkins-ci comemnting with link, you
> find it manually in Diff detail section, see screenshot
> 
> https://screenshots.firefox.com/1GG7B0bPLod3QMFg/phabricator.kde.org
> 
> Here, the "Jenkins" after the "Build 1313: Frameworks Pre-Review CI" is
> link to jenkins build. But the test was if you were able to spot it in
> first glance, and it failed. :P so yeah we will keep comments enabled
> even if it means extra emails.

Eh, not talking about that does not mean I would have not seen it ;)
Cannot remember if I did, but such a central last build status display would 
be good to have in general, so there is one defined place to look at the learn 
about the latest build report for the current state, and not having to search 
for in the timeline. So that entry being there or some other defined place 
could be taken for granted :)

Having the different build reports also in the timeline (by a comment or 
whatever other entry type phabricator allows) might be nice to have, it might 
help to compare results when trying to fix a build failure only hit/available 
on CI.


Which brings another question: how long would review build logs be kept?

I could imagine that discarding once a review is closed (discarded or 
committed) would be fine. But while a review request is open, keeping also 
build logs for older revisions (at least to a certain count) of a patch under 
review would be good to have, as sometimes one needs to compare results for 
different versions.

Cheers
Friedrich

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

[Friedrich W. H. Kossebau]

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

To: kossebau, mgallien
Cc: bcooksley, aacid, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

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

[Aleix Pol Gonzalez]

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

D10040: Add serial number and EISA ID to OutputDevice interface

[Roman Gilg]

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, &OutputDeviceInterface::serialNumberChanged,   this, 
> [this, d] { d->updateGeometry(); });
> +connect(this, &OutputDeviceInterface::eisaIdChanged, this, 
> [this, d] { d->updateGeometry(); });
>  connect(this, &OutputDeviceInterface::scaleChanged,  this, 
> [this, d] { d->updateScale(); });

needs fix

> outputdevice_interface.cpp:410
> +wl_resource_post_event(resource,
> +ORG_KDE_KWIN_OUTPUTDEVICE_GEOMETRY,
>  globalPosition.x(),

Why change? Unrelated.

> outputdevice_interface.h:50
> +//KF6 TODO - This class sends absolute garbage over the wire constantly.
> +//sendDone needs to be explicit, anything related to the geometry event 
> needs to be in a single method
> +

What do you mean with "needs to be explicit"?

REPOSITORY
  R127 KWayland

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

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: romangg, kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, 
Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D12388: Output device color curves correction

[Roman Gilg]

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, graesslin, romangg
Cc: kde-frameworks-devel, graesslin, davidedmundson, zzag, cfeck, michaelh, 
ngraham, bruns

D10040: Add serial number and EISA ID to OutputDevice interface

[David Edmundson]

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&id=38772

BRANCH
  output_changes

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

AFFECTED FILES
  autotests/client/test_wayland_outputdevice.cpp
  src/client/outputdevice.cpp
  src/client/outputdevice.h
  src/client/protocols/outputdevice.xml
  src/server/outputdevice_interface.cpp
  src/server/outputdevice_interface.h

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: romangg, kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, 
Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[David Edmundson]

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 broadcasts a change to every connected 
client...
  I need to follow that up in another patch, possibly by making it static like 
these two.

REPOSITORY
  R127 KWayland

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

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: romangg, kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, 
Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D14308: [Exe Thumbnailer] Improve icon selection algorithm

[Kai Uwe Broulik]

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 `delta`
> 
> 1. all candiates have exactly the same aspect ratio. The difference below is 
> constant, thus has no influence on the sorting. Sorting is determined by 
> `delta`alone (good).
> 2. candidates have different aspect ratios. Any candidate with a slightly 
> different aspect ratio is immediately heavily penalized, and thus discarded 
> (bad).
> 
> Think of e.g. a requested size of 96x96, and two candidates, 32x32 and 
> 128x64. The small one has a `distance` of 128, while the second one has a 
> distance 25032. I don't think that's wanted here.

I'm open to suggestions … also technically you cannot assume the icon is square

REPOSITORY
  R320 KIO Extras

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

To: broulik, #frameworks, dfaure, ngraham, pali, vonreth, antlarr
Cc: bruns

D14449: Modify device usage information

[Nathaniel Graham]

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

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

D14449: Modify device usage information

[Shubham]

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&id=38784

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

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp

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

D14308: [Exe Thumbnailer] Improve icon selection algorithm

[Stefan Brüns]

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

To: broulik, #frameworks, dfaure, ngraham, pali, vonreth, antlarr
Cc: bruns

D14308: [Exe Thumbnailer] Improve icon selection algorithm

[Anthony Fieroni]

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

To: broulik, #frameworks, dfaure, ngraham, pali, vonreth, antlarr
Cc: anthonyfieroni, bruns

D14308: [Exe Thumbnailer] Improve icon selection algorithm

[Stefan Brüns]

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, antlarr
Cc: anthonyfieroni, bruns

D10040: Add serial number and EISA ID to OutputDevice interface

[Roman Gilg]

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 copy the current setEdid pattern, which is broken ATM. 
Whenever any new client connects it broadcasts a change to every connected 
client...
  >  I need to follow that up in another patch, possibly by making it static 
like these two.
  
  
  Can you explain both points a bit more? Maybe when updating the Summary.

REPOSITORY
  R127 KWayland

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

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: romangg, kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, 
Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D14308: [Exe Thumbnailer] Improve icon selection algorithm

[Anthony Fieroni]

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, ngraham, pali, vonreth, antlarr
Cc: anthonyfieroni, bruns

D14308: [Exe Thumbnailer] Improve icon selection algorithm

[Stefan Brüns]

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 ratio of candidates differ, use the best fit
- For all subsets of (approximately) same aspect ratio, (2.) should select 
one candidate per set
- Determine which of the subsets yielded the best overall candidate
  
  The last point is difficult to answer - prefer upscaling a candidate with 
matching aspect ratio, or downscaling a nonmatching one.

REPOSITORY
  R320 KIO Extras

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

To: broulik, #frameworks, dfaure, ngraham, pali, vonreth, antlarr
Cc: anthonyfieroni, bruns

D14308: [Exe Thumbnailer] Improve icon selection algorithm

[Stefan Brüns]

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
  R320 KIO Extras

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

To: broulik, #frameworks, dfaure, ngraham, pali, vonreth, antlarr
Cc: anthonyfieroni, bruns

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

[Dominik Haumann]

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
  https://phabricator.kde.org/D14471?vs=38728&id=38796

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

AFFECTED FILES
  autotests/theme_test.cpp
  src/lib/format.cpp
  src/lib/format.h

To: dhaumann, cullmann, vkrause
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D14468: Add QVector Definition::includedDefinitions() const

[Dominik Haumann]

This revision was automatically updated to reflect the committed changes.
Closed by commit R216:3d3b4589e9e2: Add QVector 
Definition::includedDefinitions() const (authored by dhaumann).

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14468?vs=38723&id=38797

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

AFFECTED FILES
  autotests/syntaxrepository_test.cpp
  src/lib/definition.cpp
  src/lib/definition.h

To: dhaumann, vkrause, cullmann
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D14451: Add Definition::::formats()

[Dominik Haumann]

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&id=38798#toc

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14451?vs=38721&id=38798

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

AFFECTED FILES
  autotests/syntaxrepository_test.cpp
  src/lib/definition.cpp
  src/lib/definition.h

To: dhaumann, cullmann, vkrause
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D14434: add functions to access keywords

[Dominik Haumann]

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&id=38799#toc

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14434?vs=38629&id=38799

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

AFFECTED FILES
  autotests/syntaxrepository_test.cpp
  src/lib/definition.cpp
  src/lib/definition.h
  src/lib/keywordlist.cpp
  src/lib/keywordlist_p.h

To: jpoelen, #framework_syntax_highlighting, dhaumann, asemke
Cc: vkrause, cullmann, asemke, kde-frameworks-devel, kwrite-devel, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars, dhaumann

D14449: Modify device usage information

[Dominik Haumann]

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".
  
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?

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

[Shubham]

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 sense to me: Two lines 
with the distinction of "Device capacity" and "Device usage".
  >
  >   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?
  
  
  dhaumann: but that deviec capacity label seems to be redundant, because 
capacity is simply  used + 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

D14449: Modify device usage information

[Nathaniel Graham]

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 sense 
to make the user do math to find out how big their disk is.

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

[Dominik Haumann]

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 progress bar + 2 lines text behind is clearly not 
visually the best solution :-)

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

[Shubham]

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

[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

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

[David Faure]

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

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

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

[David Faure]

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

D14499: Use in-class member initialization where possible

[Dominik Haumann]

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

TEST PLAN
  make test

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  useMemberInit (branched from master)

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

AFFECTED FILES
  src/lib/context.cpp
  src/lib/context_p.h
  src/lib/contextswitch.cpp
  src/lib/contextswitch_p.h
  src/lib/definition.cpp
  src/lib/definition_p.h
  src/lib/format.cpp
  src/lib/format_p.h
  src/lib/keywordlist.cpp
  src/lib/keywordlist_p.h
  src/lib/matchresult.cpp
  src/lib/matchresult_p.h
  src/lib/repository.cpp
  src/lib/repository_p.h
  src/lib/rule.cpp
  src/lib/rule_p.h
  src/lib/state.cpp
  src/lib/state_p.h
  src/lib/textstyledata_p.h
  src/lib/themedata.cpp
  src/lib/themedata_p.h

To: dhaumann, vkrause
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D14449: Modify device usage information

[Dominik Haumann]

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 example of wrong alignment.

REPOSITORY
  R241 KIO

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

To: shubham, elvisangelaccio, ngraham, #frameworks
Cc: rkflx, dhaumann, 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

D14360: Remove custom icon selection for trash

[Nathaniel Graham]

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

> kfileplaceeditdialog.h:124
>  bool applicationLocal() const;
> +
> +/**

Remove the extra four spaces on this line please

> kfileplaceeditdialog.h:127
> + * @returns check for icon's editabilty
> + */
> +bool canEditIcon() const;

This is a public function so please add `@since 5.49`

REPOSITORY
  R241 KIO

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

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

D14360: Remove custom icon selection for trash

[Nathaniel Graham]

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: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14308: [Exe Thumbnailer] Improve icon selection algorithm

[Stefan Brüns]

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 = (1.0 * desiredHeight) / height;

// clamp to the lower of the two scales
// also clamp to one, as scaling up adds no effective
// samples, only interpolated samples
auto sampleScale = min(1, min(xscale, yscale));

// number of effective source samles in the target
auto effectiveSamples = width * height * scale * scale;
// scale down another time, to account for loss of fidelity when
// using a downscaled image, biases towards smaller downscaling ratios
effectiveSamples *= scale;

return targetSamples - effectiveSamples;
}

REPOSITORY
  R320 KIO Extras

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

To: broulik, #frameworks, dfaure, ngraham, pali, vonreth, antlarr
Cc: anthonyfieroni, bruns

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

[Albert Astals Cid]

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
  https://phabricator.kde.org/D14473?vs=38730&id=38815

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

AFFECTED FILES
  autotests/klocalizedstringtest.cpp
  autotests/klocalizedstringtest.h
  autotests/po/ca/ki18n-test.po
  src/kcatalog.cpp

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

D14502: Reuse function that already does the same

[Aleix Pol Gonzalez]

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

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

AFFECTED FILES
  src/klocalizedstring.cpp

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

D14503: Android: also fall-back to using QLocale

[Aleix Pol Gonzalez]

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 defined on Android and
  QLocale is supported.

TEST PLAN
  Now Android detects some language

REPOSITORY
  R249 KI18n

BRANCH
  master

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

AFFECTED FILES
  src/klocalizedstring.cpp

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

D14504: Save few string allocations

[Aleix Pol Gonzalez]

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. Not a big deal
  anyway.

TEST PLAN
  Tests pass

REPOSITORY
  R249 KI18n

BRANCH
  master

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

AFFECTED FILES
  src/klocalizedstring.cpp

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

D14505: Sync set/send/update methods

[David Edmundson]

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 binds we would incorrectly send an
  EDID/uuid/enabled update to every client.
  
  This syncs every property into following the same set/send/update
  pattern everywhere.

TEST PLAN
  Existing unit tests

REPOSITORY
  R127 KWayland

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

AFFECTED FILES
  src/server/outputdevice_interface.cpp

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

D14501: Top-align labels in properties dialog

[Christoph Feck]

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.
  
  On remote folders, it looks even worse, because the text line is empty, and 
so the "Calculate" button looks disconnected from the Size label.

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

D14360: Remove custom icon selection for trash

[Christoph Feck]

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 not editable would be nice.

Also, I would have named it 'isIconEditable()'.

REPOSITORY
  R241 KIO

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

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

D14501: Top-align labels in properties dialog

[Nathaniel Graham]

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, michaelh, ngraham, bruns

D14449: Modify device usage information

[Nathaniel Graham]

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?
  
  
  I'm fine with that. A simple way to make sure the alignment is correct is to 
put all the strings that should be on a second line in their own cell in the 
`gridView`.

REPOSITORY
  R241 KIO

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

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

D14360: Remove custom icon selection for trash

[Shubham]

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&id=38824

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

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp
  src/filewidgets/kfileplaceeditdialog.h

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

Re: Pre-review CI

[Bhushan Shah]

On Mon, Jul 30, 2018 at 11:50:48AM +0200, Friedrich W. H. Kossebau wrote:
> More advantages with parallel builds:
> * bigger projects, where building takes some time (krita, kdevelop, plasma-*, 
> etc) and where sometimes patches are reviewed almost in sync, parallel builds 
> mean quicker feedback
> * when doing fix-up for patches which fail on a platform, getting quicker 
> feedback would be also good
> * parallel builds might also mean easier report for results on the different 
> platforms in the jenkins page, so one can quickly see on which platform there 
> are issues (and which experts might be needed)
> * a platform being broken on normal CI for some unrelated reason (outdated 
> deps, platform issues) will also mean the review build breaks there, in that 
> case any later platforms in the single job build would not be reached for 
> review feedback
> * platform-specific issues (like missing includes) which need platform 
> experts 
> will result in a fail before reaching other issues (like regressions in 
> tests), having parallel builds would allow the non-build failing platforms to 
> also run the tests and allow the developer to already investigate the test 
> regression, while waiting for the platform experts to help out with the 
> platform specific issues
> 
> So unless we are short of resources on CI, I really would favour separate 
> builds, to always get feedback for all platforms, instead of doing some try & 
> error walk through all platforms one by one, with the others having to wait 
> for the earlier (and respective people to sort out things there).
> 
> Let's make sure things can be worked in parallel where possible, and as quick 
> as possible, given most FLOSS contributors only have small time snippets to 
> do 
> things.

Your arguments makes sense perfectly.

> Which brings another question: how long would review build logs be kept?
> 
> I could imagine that discarding once a review is closed (discarded or 
> committed) would be fine. But while a review request is open, keeping also 
> build logs for older revisions (at least to a certain count) of a patch under 
> review would be good to have, as sometimes one needs to compare results for 
> different versions.

Builds will stay on CI as long as the review request is open, also we
are thinking of the "cleanup" at regular period to purge the review
request builds which had no updates in "some period".

Thanks

-- 
Bhushan Shah
http://blog.bshah.in
IRC Nick : bshah on Freenode
GPG key fingerprint : 0AAC 775B B643 7A8D 9AF7 A3AC FE07 8411 7FBC E11D


signature.asc
Description: PGP signature

Re: Pre-review CI

[Ben Cooksley]

On Mon, Jul 30, 2018 at 9:50 PM, Friedrich W. H. Kossebau
 wrote:
> 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 don't care about the pre-review CI jobs as long as they
>> are passing, but in case of fail, yes you might have to look at long
>> log depending on where failure was, in first platform or last one.
>
> So this is a use-case which should be optimized for, given it is an expected
> to hit hot-path of developer workflow :)
>
>> > compare to non-review build jobs, I would assume. And having jobs separate
>> > also means one gets results for any platforms, does not stop on the first
>> > failing?
>>
>> Yes, but it also means that if there is obvious mistake in review, all
>> will fail, insteaad of bailing out earlier.
>
> More advantages with parallel builds:
> * bigger projects, where building takes some time (krita, kdevelop, plasma-*,
> etc) and where sometimes patches are reviewed almost in sync, parallel builds
> mean quicker feedback
> * when doing fix-up for patches which fail on a platform, getting quicker
> feedback would be also good
> * parallel builds might also mean easier report for results on the different
> platforms in the jenkins page, so one can quickly see on which platform there
> are issues (and which experts might be needed)
> * a platform being broken on normal CI for some unrelated reason (outdated
> deps, platform issues) will also mean the review build breaks there, in that
> case any later platforms in the single job build would not be reached for
> review feedback
> * platform-specific issues (like missing includes) which need platform experts
> will result in a fail before reaching other issues (like regressions in
> tests), having parallel builds would allow the non-build failing platforms to
> also run the tests and allow the developer to already investigate the test
> regression, while waiting for the platform experts to help out with the
> platform specific issues
>
> So unless we are short of resources on CI, I really would favour separate
> builds, to always get feedback for all platforms, instead of doing some try &
> error walk through all platforms one by one, with the others having to wait
> for the earlier (and respective people to sort out things there).

At the moment we can do a maximum of 3 Windows builds and 3 FreeBSD
builds at any given time.
Depending on how things go, especially at peak time, we may need to
look into additional resources for the system.

>
> Let's make sure things can be worked in parallel where possible, and as quick
> as possible, given most FLOSS contributors only have small time snippets to do
> things.

That seems reasonable enough, however executing them in parallel will
mean that the system will be quite chatty in the case of failures, as
each platform will be represented by a different build plan on the
Phabricator side (ie. you won't get one comment saying it failed,
you'll get N comments, where N is the number of platforms that
repository is enabled for)

>
>> > > - Should we send out comment for failure and success? Or is it easier to
>> > >
>> > >   figure out the console log link without the comment? See linked review
>> > >   for example [1].
>> >
>> > [1] -> [2] here.
>> >
>> > What do you mean exactly by "send out comment for failure and success"?
>> > More emails? (Please not). That example works fine with me, but not sure
>> > what the alternative is?
>>
>> The alternative being, instead of jenkins-ci comemnting with link, you
>> find it manually in Diff detail section, see screenshot
>>
>> https://screenshots.firefox.com/1GG7B0bPLod3QMFg/phabricator.kde.org
>>
>> Here, the "Jenkins" after the "Build 1313: Frameworks Pre-Review CI" is
>> link to jenkins build. But the test was if you were able to spot it in
>> first glance, and it failed. :P so yeah we will keep comments enabled
>> even if it means extra emails.
>
> Eh, not talking about that does not mean I would have not seen it ;)
> Cannot remember if I did, but such a central last build status display would
> be good to have in general, so there is one defined place to look at the learn
> about the latest build report for the current state, and not having to search
> for in the timeline. So that entry being there or some other defined place
> could be taken for granted :)
>
> Having the different build reports also in the timeline (by a comment or
> whatever other entry type phabricator allows) might be nice to have, it might
> help to compare results when trying to fix a build failure only hit/available
> on CI.
>
>
> Which brings another question: how long would review build logs be kept?
>
> I could imagine that discarding once a review is closed (discarded or
> committed) would be fine. But while a review requ