Re: KF6 meeting notes 2021-04-24

[Dominik Haumann]

Hi Luigi,

with respect to the reminder of the weekly meetings: It might make
sense to send such a reminder *before* these meetings and not after
:-)

Besides that, thanks for sending these minutes to keep us all informed.

Best regards
Dominik

On Sat, Apr 24, 2021 at 4:19 PM Luigi Toscano  wrote:
>
> Hi all,
> we had more Plasma people around today, so the work focused on Plasma notes
> (some of which turned out to not be really KF6.0 blockers).
>
> Before going into the notes, I'd like to remind everyone that the meeting
> happens every Saturday at 15.00 CEST (currently 13.00 GMT) and it uses the
> same infrastructure (BBB) of the KF6 sprint:
> https://community.kde.org/Sprints/KF6/2021Virtual
>
> Now, the notes.
>
> ---
> https://phabricator.kde.org/T11587 "Investigate making KColorScheme tier1"
> Very useful as it touches several dependencies, but no people around for this,
> let's move it to another meeting.
>
>
> https://phabricator.kde.org/T13889 "Plasma::PluginLoader cleanup"
> -> not really KF6, just Plasma. Moved to backlog.
>
>
> https://phabricator.kde.org/T12611 "Make Breeze a framework and relocate all
> of its Plasma theme stuff (e.g. wallpaper) to a different repo still on the
> Plasma release schedule"
> -> no need for input, move to "move to KF6 branching"
>
>
> https://phabricator.kde.org/T13467 "Theming in Plasma 6"
> A lot of discussions, agreement in place, just a lot of work.
> But not blocking for KF6.0.
> The only task related to KF6 is the creation/splitting of a SVG library and
> its addition to KF6, which is tracked by https://phabricator.kde.org/T12109
> -> remove the KF6 tag
>
>
> https://phabricator.kde.org/T12105 "Plasma Framework: Discussion Configuration
> dialogs"
> It's a cleanup task, Plasma-only
> -> will be updated and KF6 tag removed
>
>
> https://phabricator.kde.org/T12117 "Plasma framework: qml imports plasma Core"
> Clean up in the imports of plasma-frameworks. Relevant for KF6 (and the part
> of plasma-frameworks which will stay in KF6).
> -> improve the description, move to "at branching time"
>
>
> https://phabricator.kde.org/T12542 "Fix circular dependency of
> applications.menu in KService and plasma-workspace"
> Still need some discussion, but no  one around remembers the details right 
> now.
>
>
> https://phabricator.kde.org/T11903 "KWayland for KF6"
> To be reevaluated now that the we have split some wayland stuff in separate
> repositories.
> Client part should be ported away from KWayland.
> -> no "needs input", the direction "port away from kwayland client" is clear
> (see last comment), move to "backlog"
>
>
> There were some questions about a proposed tasks which affects KIconLoader to
> switch the default to 24x24  (instead of 22x22)
> https://phabricator.kde.org/T14397
> -> people are going to look which code is potentially affected this change. On
> the Frameworks side, it only affects the "when" to switch the size
>
>
> For the next meetings: there are tasks which would benefit from having around:
>
> - Kai Uwe (https://phabricator.kde.org/T12536 ,
> https://phabricator.kde.org/T11833 , https://phabricator.kde.org/T11875 ,
> https://phabricator.kde.org/T12008 )
>
> - more Davids (https://phabricator.kde.org/T12275,
> https://phabricator.kde.org/T12542, etc etc etc)
>
> so if you match the description above and read this, please try to be around
> next time!
>
> Ciao
> --
> Luigi

Re: KDE review for KWeatherCore

[Dominik Haumann]

Hi Han,

let's have this discussion public on kde-core-devel and kde-framworks,
so other can contribute.

On Sat, Jan 2, 2021 at 1:29 PM hanyoung  wrote:
>
> Hi Dominik,
>
> Thank you for your time. This is my first time writing a library, I tried my 
> best by looking
> at existing KDE frameworks source code and articles on KDE wiki. Obvious 
> KWeatherCore
> is far from polished. However, there are still something I want to explain.


There is always a first time, so welcome :-)

>
> > - #pragma once: It's used in all header files.
> I've once encountered name conflict with one KDE frameworks and that took me 
> half a hour
> to debug. I want to #pragma once to avoid this kind of issue to appear, since 
> most modern
> compiler supports this feature.


Yes, I am aware that essentially all compilers support it. I was just
mentioning the status quo.

>
> > 1. The first sentence in doxygen is automatically he brief version.
> It's on QHC documentation, so I just copied it. I guess it's fine since it 
> doesn't do any harm.


It does harm in the sense that it adds visual noise. That is why I
suggested to remove it.
Other example: Many developers (outside of KDE) use void parameters
such as in void foobar(void);
This (void) is not needed either, it just adds noise. It's the exact
same thing here with doxygen.
You don't need it. So why add it?

>
> > 2. Obviously, GeoTimezone is not a complete documentation :-)
> Can't argue with that :P
> Actually, some problems with this class are all partially related to this.
>
> > 5. Copyright:
> Didn't know about that, my apologies. Will definitely change that.
>
> > 6. File naming.
> Didn't notice that. My bad.
>
> > 7. Constructors.
> It has another constructor takes zero arguments. And this class provides all 
> setters and getters.
> The reason this "one for all" constructor exists is for internal convenience 
> use. And I thought
> export it does no harm.

Let me try to explain this a bit more in depth.

When back in the old days Trolltech released Qt4, they broke a lot of API.
One reason for breaking API was the focus on good and readable API.
In fact, Mathias Ettrich even held a talk to the KDE audience in Ludwigsburg,
Germany in 2004 why API design matters a lot. The link I mentioned previously
(https://doc.qt.io/archives/qq/qq13-apis.html) essentially contains
all the points
back from this talk. Qt tries to stick to these rules, and KDE does as well.

Therefore I'd like to emphasize this again: Constructors that take
e.g. 8 arguments
and most of them are of type double will lead to unreadable code.
Theses constructors
also will not work, once you extend the class with  function and
member. Then, you have
one missing member that is not included in the constructor. So this
approach is not open
to extensions. Or it short: that is bad API design :-)

Let me phase it differently: Can we find an API that is even better?
You have a default constructor - fair enough. But I believe it can be better,
that is why I suggested the version that takes only a QDate, since you
cannot misuse it,
and the API is rather explicit. And the most important property of a
forecast is the date,
since otherwise the information is useless.

> > 8. Wrong order of API documentation:
> My bad.
>
> > 9. Pass non primitive data types (pod) by const reference:
> I use std::move, so pass by value means one copy and one move for lvalue, one 
> move for rvalue.
> Const reference costs one copy for everything. Pass by value saves me one 
> rvalue reference overload function.
> Consider that I have so many setters, it's justifiable.

I saw and understood this, yes. But this approach still has its
drawbacks. Let me explain.
When you use std::move internally, then you don't have any performance
penalty for now,
since the version with const ref would also do a copy later.

But let's say you need to internally change the implementation such
that you don't
need a copy anymore. Instead you just read this parameter. Then, you
still have the
copy in the public API and you cannot change it anymore due to binary
compatibility.

Or in other words: Currently you force API design decisions on the
public API that
rely on some internal implementation details. That is always a bad idea.

I hope what I wrote is understandable :-)

> > 10. Mixup of documentation + unexport macro.
> I can definitely improve that.
>
> > I assume the weatherDescription is a possibly user visible string?
> Thank you, I just noticed that default value for weatherIcon and 
> weatherDescription are reversed. My bad.
>
> Thanks again for taking time to look through my terrible code base.

It's not terrible, it just needs several rounds of reviews to reach
the quality to be
included in kde-frameworks.

> Happy new year!

Thanks, same to you!

Best regards
Dominik

Re: KDE review for KWeatherCore

[Dominik Haumann]

Hi Han,

just having a quick glance at the code, and I feel like there is a lot of
polishing
missing. I'll give some examples, but this is by no means a complete review,
and I'll not comment on technical aspects at all, since this is not the
domain
of my expertise.

- #pragma once: It's used in all header files. I am not sure whether we have
any policy here, but indeed it's almost not used at all in KDE Frameworks.
Maybe someone else can clarify.

- API documentation largely incomplete: For instance, looking at
geotimezone.h,
we see

/**
 * @brief GeoTimezone
 * @param lat latitude
 * @param lon longitude
 * @param parent
 */
GeoTimezone(double lat, double lon, QObject *parent = nullptr);

1. The first sentence in doxygen is automatically he brief version. You can
omit @brief
everywhere. Its just noise.

2. Obviously, GeoTimezone is not a complete documentation :-)

3. Try to avoid abbreviations. That is: Name the parameters latitude and
longitude
instead of lat and lon. Explaining a parameter with the name itself is also
no good
practice. Better would be: @param latitude The latitude used in
geographical timezone. or so...

Q_SIGNALS:
/**
 * @brief finished
 * @param timezone
 */
void finished(const QString );

4. What is finished? It seems to me that this API is not intuitive :-)
And again: the API documentation is essentially missing.

/**
 * @brief networkError encounted network error
 */
void networkError();

Better would be networkErrorOccured() or so, to stress the fact that this
is a signal.

5. Copyright:
 * Copyright 2020 Han Young 
 * Copyright 2020 Devin Lin 
 * SPDX-License-Identifier: LGPL-2.0-or-later

I think nowadays we encourange using SPDX-FileCopyrightText to list the
author.
This can appear multiple times, from what I understand.
https://community.kde.org/Policies/Licensing_Policy#SPDX_Statements


6. File naming.
We have dailyforecast.h, but the class is called DailyWeatherForecast.
It's good practice to name the header files exactly like the class names
for public API.

7. Constructors. Let's have a look at this:
DailyWeatherForecast(double maxTemp,
 double minTemp,
 double precipitation,
 double uvIndex,
 double humidity,
 double pressure,
 QString weatherIcon,
 QString weatherDescription,
 QDate date);

Using this looks like this: DailyWheaterFoecast forecast(0, 10, 35, 80,
1023, "sun", "Sunny Weather", ...);

Please read "The Convenience Trap" here:
https://doc.qt.io/archives/qq/qq13-apis.html
In other words: Try to avoid constructors that take many many arguments to
define the object in one go.
What is much more readable is to have 3-4 lines more code, but at least it
can be understood:
DailyWheterhForecast forecast(QDate(2020, 12, 31));
forecast->setIcon("sun");
forecast->setDescription("Nice beach weather");
forecast->setTemperatureRange(2, 27);
...


8. Wrong order of API documentation:
/**
 * Return a QJsonObject that can be converted back with
 * DailyWeatherForecast::fromJson
 */
~DailyWeatherForecast();
QJsonObject toJson();

Here, the virtual destructor will get the API documentation of toJson().
Obviously not what is wanted.


9. Pass non primitive data types (pod) by const reference:
void setWeatherIcon(QString icon);
void setWeatherDescription(QString description);
void setDate(QDate date);

const QString &
const QDate &

10. Mixup of documentation + unexport macro.
/**
 * @brief return maximum temperature
 * @return maximum temperature, this value is initialized to the
smallest
 * value double can hold
 */
KWEATHERCORE_NO_EXPORT void setJsDate(const QDateTime );
double maxTemp() const;

Here, setJsDate() will get the API documentation of maxTemp(). That is not
what you want.
Typically, having to explicitly hide a symbol (_NO_EXPORT) is showing
design issues.
I'd recommend to move this function to the pimpl class, and if you need
internally access to
this function, then move your pimpl class to e.g. dailyweatherforecast_p.h.

This is just by looking at the first two header files.

Looking at WeatherForecast.cpp:

double maxTemp = std::numeric_limits::min();
double minTemp = std::numeric_limits::max();

Initializing the temperature to 0, 0, sounds a bit more sane to me, but
that is disputable I guess.

QDate date = QDate();
The default constructor will be called automatically, better would be
simply:
QDate date;

bool isNull(); The semantics of isNull() is a bit weird. Typically, the
naming of bool isValid() is
used in Qt and therefore better.

I assume the weatherDescription is a possibly user visible string?
Currently, this is initialized
to QString weatherDescription =
QStringLiteral("weather-none-available"); So possibly
the 

D29789: Make text always align with font base line

[Dominik Haumann]

dhaumann added a comment.


  I like this patch. In fact, I observed over the past years again and again 
that sometimes, especially if chinese or similar letters are included, the 
baseline is wrong in Kate, leading to much more overpainting that needed.
  If this patch fixes this, then I'm all for it. Or let's put it like this: The 
current implementation is wrong, this patch looks less wrong than the current 
state :-)
  
  +1

REPOSITORY
  R39 KTextEditor

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

To: xuetianweng, rjvbb, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D27844: Store and fetch complete view config in and from session config

[Dominik Haumann]

dhaumann added a comment.


  I suggest to revert, and send a notification with the change to 
kde-distro-packa...@kde.org to avoid that many users break their configuration.

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, loh.tar, cullmann, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D29735: Assign "Identifier" attribute to opening double quote instead of "Comment"

[Dominik Haumann]

dhaumann added a subscriber: cullmann.
dhaumann added a comment.


  @cullmann could you integrate this?

REPOSITORY
  R216 Syntax Highlighting

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

To: bewuethr, #framework_syntax_highlighting, dhaumann
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, 
cblack, gennad, bmortimer, domson, michaelh, genethomas, ngraham, bruns, 
demsking, vkrause, sars

D29735: Assign "Identifier" attribute to opening double quote instead of "Comment"

[Dominik Haumann]

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


  Good catch :) please commit

REPOSITORY
  R216 Syntax Highlighting

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

To: bewuethr, #framework_syntax_highlighting, dhaumann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, 
gennad, bmortimer, domson, michaelh, genethomas, ngraham, bruns, demsking, 
cullmann, vkrause, sars

D25339: update lineHeight if boundingRect indicates a larger value.

[Dominik Haumann]

dhaumann added a comment.


  In D25339#666832 , @cullmann wrote:
  
  > Looking at the code, might it make more sense to just move away from the 
fixed height we have?
  
  
  Yes please. Instead of hacks and options, which will make the code and the UI 
harder to use, the correct fix is to switch to variable line heights.
  
  Of course you need to touch many places, but this approach is so much better 
than hacks. Please give it a try! :-)

REPOSITORY
  R39 KTextEditor

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

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D28039: optimize dynamic regex matching

[Dominik Haumann]

dhaumann added a comment.


  Thanks yes, maybe you can add a comment to the skipOffset, Christoph :)

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  master

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

To: cullmann, dhaumann, vkrause, nibags
Cc: mwolff, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, 
GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D28039: optimize dynamic regex matching

[Dominik Haumann]

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


  I guess this is OK, but the concept of a "skip offset" is a bit fuzzy to me.

INLINE COMMENTS

> rule_p.h:126
> +
> +protected:
> +bool m_dynamic = false;

I dislike this protected hack :-)

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  master

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

To: cullmann, dhaumann, vkrause, nibags
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, GB_2, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D28015: extend indexer to detect dynamic=true regexes that have no place holders to adapt

[Dominik Haumann]

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


  I guess in a followup commit the reported issues will be fixed? :-)

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  master

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

To: cullmann, nibags, dhaumann, vkrause
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, GB_2, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D27928: [VIM Mode] Add g g commands

[Dominik Haumann]

dhaumann added a comment.


  The intimidating one :)

REPOSITORY
  R39 KTextEditor

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

To: gikari, #kate, #frameworks
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, 
GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, sars

D27916: Add Overpass QL highlighting

[Dominik Haumann]

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


  Almost good, please fix + ship.

INLINE COMMENTS

> overpassql.xml:7
> +-->
> + extensions="*.overpassql" author="Volker Krause (vkra...@kde.org)" 
> license="MIT">
> +  

For dsPreprocessor I think you need kateversion= 5.0.

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  master

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

To: vkrause, dhaumann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, 
GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, sars

D27929: CMake syntax fixes

[Dominik Haumann]

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


  Thanks!

REPOSITORY
  R216 Syntax Highlighting

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

To: turbov, #framework_syntax_highlighting, dhaumann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, 
GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, sars

D27928: [VIM Mode] Add g g commands

[Dominik Haumann]

dhaumann added a comment.


  I like the patch, but please add a unit test before we commit this. See 
https://github.com/KDE/ktexteditor/tree/master/autotests/src/vimode
  Could you add one? :)

REPOSITORY
  R39 KTextEditor

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

To: gikari, #kate, #frameworks
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, 
GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, sars

D27844: Store and fetch complete view config in and from session config

[Dominik Haumann]

dhaumann added a comment.


  I guess we can give this a try. As I understand, though, with this patch you 
will never be able to use e.g. a global zoom once you change the zoom of a 
view. This was different before this patch.
  
  Which brings me to another point: e.g. zoom should simply zoom all documents, 
so zoom should not be done per view imho. In particular, with this patch you 
can end up with a different font size for each view, which is certainly not 
what a user wants.

REPOSITORY
  R39 KTextEditor

BRANCH
  fullviewconfigsessionsupport

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

To: kossebau, #kate, loh.tar, cullmann, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, GB_2, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kateviewhelpers.cpp:1963
> You should keep devicePixelRatioF calls

This is true. But in this case a non-answer: Maybe QIcon::paint does it correct 
as well. In other words, the code can very likely be improved, but also ok as 
is.

REPOSITORY
  R39 KTextEditor

BRANCH
  addmarkinterfacev2

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

To: kossebau, #kate, #kdevelop, dhaumann
Cc: anthonyfieroni, dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, 
LeGast00n, cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, 
sars

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Dominik Haumann]

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

REPOSITORY
  R39 KTextEditor

BRANCH
  addmarkinterfacev2

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

To: kossebau, #kate, #kdevelop, dhaumann
Cc: anthonyfieroni, dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, 
LeGast00n, cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, 
sars

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Dominik Haumann]

dhaumann added a comment.


  Ok, then I am fine with this. Maybe add a KF6 task to the KF6 board?

INLINE COMMENTS

> kossebau wrote in katedocument.h:86
> See the warning in the API docs, nobody should rely on this private API, so 
> the BIC here is okay.
> The class is exported for the unit tests basically.
> Also is the header file not installed, so not really available outside.

Friedrich is correct. We have no issue here.

> katedocument.h:586
>  QHash m_marks;
> -QHash m_markPixmaps;
> +QHash m_markIcons; // QPixmap or QIcon, KF6: remove 
> QPixmap support
>  QHash m_markDescriptions;

Better turn " Remove pixmap support" into use QIcon only.

> katesearchbar.cpp:906
>  if (!m_highlightRanges.empty()) {
> -KTextEditor::MarkInterface *iface = 
> qobject_cast(m_view->document());
> +KTextEditor::MarkInterfaceV2 *iface = 
> qobject_cast(m_view->document());
>  if (iface) {

maybe use `auto` here.

> kossebau wrote in kateviewhelpers.cpp:1963
> Possibly `QIcon::paint()` might be also working here as wanted? Needs someone 
> with HiDPI to check if all things behave as wanted. The old code with all the 
> `devicePixelRatioF()` made me change not too much here.

I believe you can check yourself with proper environment variables, or?

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: anthonyfieroni, dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, 
LeGast00n, cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, 
sars

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Dominik Haumann]

dhaumann added a comment.


  Definitely a +1 for using a QIcon here.
  
  Still, I wonder whether we should postpone adding the MarkInterfaceV2 until 
the KF6 branch. Then we have it properly fixed in KF6.
  
  Or do you think we also need this in KF5 days to properly support e.g. hidpi ?

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, 
GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, sars

D27285: Add left/right indent fill (as opposed to left-only), extend indent lines to broken lines

[Dominik Haumann]

dhaumann added a subscriber: cullmann.
dhaumann added a comment.


  Ok, I now fully got this, thanks for the explanation. And indeed without any 
left fill visual feedback is missing.
  
  What I wonder is how much of this needs to be configurable. We could also 
just make the left fill of 4 default or so.
  
  In any case, I think we should go forward with this patch. @cullmann your 
opinion?

REPOSITORY
  R39 KTextEditor

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

To: eudoxos, #vdg
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, 
cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, sars

D27285: Add left/right indent fill (as opposed to left-only), extend indent lines to broken lines

[Dominik Haumann]

dhaumann added a comment.


  No offense meant, but even with the screenshots I still have no idea what 
this is about :)
  
  Can you add a before/after screenshot so we can see the visual difference?
  
  Also, the "fill left" and "fill right" wording is new, and not intuitively 
understandable for me.
  
  Can we do better?

REPOSITORY
  R39 KTextEditor

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

To: eudoxos, #vdg
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, 
GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, sars

D27238: Add an option to dynamic-break inside words

[Dominik Haumann]

dhaumann added subscribers: cullmann, dhaumann.
dhaumann added a comment.


  I think this patch looks good.
  
  @cullmann: this originates from 
https://www.reddit.com/r/kde/comments/ey4158/comment/fgoycn4?context=3
  Any opinion?

REPOSITORY
  R39 KTextEditor

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

To: eudoxos
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, cent, rrosch, 
LeGast00n, cblack, szutmael, GB_2, domson, michaelh, ngraham, bruns, demsking, 
head7, kfunk, sars

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

[Dominik Haumann]

dhaumann added a comment.


  Better :) with corners I mean the 1-3 pixels left due to the rounding 
corners. These pixels were once also drawn as background although they are 
outside of the frame. It may be a minor detail, but imho such details are 
important. But indeed, the screenshots look good.
  
  Next test: open a file in kate. Now either change the file externally or 
delete it. Kate should show an animated KMessageWidget. Does it also correctly 
work with a small kate? I.e. small width of kate window?

INLINE COMMENTS

> kmessagewidget.cpp:317
> +constexpr float alpha = 0.2;
> +const auto parentWidget = qobject_cast(parent());
> +const QColor parentWindowColor = (parentWidget ? parentWidget->palette() 
> : qApp->palette()).color(QPalette::Window);

Can't you use parentWidget()?
See: https://doc.qt.io/qt-5/qwidget.html#parentWidget

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  no-css (branched from master)

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

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

D26697: Message-related methods: Use more member-function-pointer-based connect

[Dominik Haumann]

dhaumann added a comment.


  In two days we have the next KDE Frameworks tag. I'd have preferred a commit 
on Sunday ;)

REPOSITORY
  R39 KTextEditor

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

To: kossebau, dhaumann, cullmann
Cc: cullmann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, GB_2, 
domson, michaelh, ngraham, bruns, demsking, sars, dhaumann

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

[Dominik Haumann]

dhaumann added a comment.


  Can you provide screenshots of more styles? :-)

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  no-css (branched from master)

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

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

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

[Dominik Haumann]

dhaumann added a comment.


  Does that also work in Kate for floating messages like when the search wraps? 
What I want to know is whether the corners behind the green frame are 
transparent in this case, or whether the corners are painted solid. If I 
remember correctly, these kind of bugs were the reason to use Qt StyleSheets. 
And it must work with all styles.
  
  Please do NOT yet commit, especially since we'd only have two days of testing 
period: Saturday, 1st of February is the next frameworks tag.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  no-css (branched from master)

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

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

D26903: allow programLogo property to be a QIcon, too

[Dominik Haumann]

dhaumann added a comment.


  Looks good to me, and very much in line with the other `canConvert` 
statements before.

REPOSITORY
  R263 KXmlGui

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

To: cullmann
Cc: dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26812: Variable expansion: Add variable PercentEncoded

[Dominik Haumann]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:b0055e8476bb: Variable expansion: Add variable 
PercentEncoded (authored by dhaumann).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26812?vs=74032=74034

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

AFFECTED FILES
  autotests/src/variable_test.cpp
  src/utils/katevariableexpansionmanager.cpp

To: dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D26811: Fix crash in variable expansion (used by external tools)

[Dominik Haumann]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:22fa707eb7e2: Fix crash in variable expansion (used by 
external tools) (authored by dhaumann).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26811?vs=74031=74033

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

AFFECTED FILES
  autotests/src/variable_test.cpp
  src/utils/katevariableexpansionhelpers.cpp

To: dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D26812: Variable expansion: Add variable PercentEncoded

[Dominik Haumann]

dhaumann created this revision.
dhaumann added a reviewer: cullmann.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
dhaumann requested review of this revision.

REVISION SUMMARY
  This can be used to encode the input text such that it can
  be used in URIs. This is useful e.g. for the external tool
  "Google Selected Text", where '&' and similar characters
  are problematic in the url.
  
  CCBUG: 416509

TEST PLAN
  make && make test

REPOSITORY
  R39 KTextEditor

BRANCH
  variable-percent-encoded

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

AFFECTED FILES
  autotests/src/variable_test.cpp
  src/utils/katevariableexpansionmanager.cpp

To: dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D26811: Fix crash in variable expansion (used by external tools)

[Dominik Haumann]

dhaumann created this revision.
dhaumann added a reviewer: cullmann.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
dhaumann requested review of this revision.

REVISION SUMMARY
  findClosing() is already called with +2 to account for the
  starting %{ characters. So we start at the given pos and
  increment it only after the while loop. Unbalanced cases
  of '{' and '}' are now also unit tested.

TEST PLAN
  make && make test

REPOSITORY
  R39 KTextEditor

BRANCH
  fix-variable-expansion

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

AFFECTED FILES
  autotests/src/variable_test.cpp
  src/utils/katevariableexpansionhelpers.cpp

To: dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D25698: New query mechanism for applications: KApplicationTrader

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> kapplicationtrader.cpp:87
> +KService::List::iterator it = lst.begin();
> +while (it != lst.end()) {
> +KService::Ptr serv = *it;

I would prefer the std::erase(std::remove_if(...), ...end()); idiom here.

Assuming the list is a vector this will be much faster, or do you have to 
preserve the order? I fear I know the answer :-)

> kapplicationtrader.h:48
> +{
> +using FilterFunc = std::function;
> +

Please document this typedef so that FilterFunc will be clickable in the 
generated API documentation.

> kapplicationtrader.h:80
> + *
> + * This a convenience methode for queryByMimeType(mimeType).at(0), with 
> a check for empty.
> + *

method without training e.

> kapplicationtrader.h:90
> +/**
> + * Returns true if @p pattern matches a subsequence of the string @p 
> text.
> + * For instance the pattern "libremath" matches the text "LibreOffice 
> Math", assuming

Maybe mention when this function is useful? For me it looks like a private 
helper function. Why is it public?

REPOSITORY
  R309 KService

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

To: dfaure, broulik, mart, vkrause, nicolasfella, aacid, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26779: Updates nasm.xml with the latest instructions

[Dominik Haumann]

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


  Looks good to me. Can you commit or shall we push this for you?

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  nasm (branched from master)

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

To: meganomic, #framework_syntax_highlighting, dhaumann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, GB_2, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars

D26755: [WIP] KMessageWidget: Set widget height on resize event

[Dominik Haumann]

dhaumann added a comment.


  Well, it definitely would be very nice if you find a patch that fixes this.

REPOSITORY
  R236 KWidgetsAddons

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

To: SGOrava, #frameworks
Cc: anthonyfieroni, dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26755: [WIP] KMessageWidget: Set widget height on resize event

[Dominik Haumann]

dhaumann resigned from this revision.
dhaumann added a comment.
This revision now requires review to proceed.


  In that case I withdraw my review for now. I can only speak for Kate that I 
could not find a regression with my tests. But if you find other regressions, 
then this is likely not correct.
  
  PS: KMessageWidget is a very unfortunate widget in this sense: We try in the 
QWidgets world to have nice animations with contents that resizes correctly. To 
do the animations, we use fixed sizes, however, at the same time the host 
application's size constraints could change, but if KMessageWidget is still in 
its animation phase, the wrong old sizes are used for the animation etc... That 
said, KMessageWidget is likely fundamentally broken, and it needs very careful 
testing to make sure we don't introduce regressions. That's why I did the tests 
for Kate/KTextEditor. But of course, also my tests test just a very limited 
amount of our reald-world scenarios.

REPOSITORY
  R236 KWidgetsAddons

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

To: SGOrava, #frameworks
Cc: anthonyfieroni, dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26755: KMessageWidget: Set widget height on resize event

[Dominik Haumann]

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


  I just tested with KTextEditor's messagetest and KTextEditor's usage of 
KMessageWidget in the top and bottom bar, and all this looks still good.
  
  +1 from my side. Another review would be welcome, though :-) Anyone?

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  more-proper-height (branched from master)

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

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

D26755: KMessageWidget: Set widget height on resize event

[Dominik Haumann]

dhaumann added a comment.


  Since KTextEditor uses KMessageWidget extensively we have to make sure the 
unit test messagetest still passes with this change.

REPOSITORY
  R236 KWidgetsAddons

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

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

D26704: KateMessageWidget: remove unused event filter installation

[Dominik Haumann]

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


  I guess this change is correct :-)

REPOSITORY
  R39 KTextEditor

BRANCH
  removeunusedeventfilter

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

To: kossebau, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D26560: Fix selection printing

[Dominik Haumann]

dhaumann added a comment.


  That looks already better: If a line is wrapped and on the same page, then 
only the selected text is printed. It seems there are still corner cases.
  
  1. Create a document with just one long line that wraps over two printed 
pages. I this case, I am not able to print only the selected text properly.
  2. Say you have a line that wraps over e.g. 5 visual lines. And you just want 
to print the visually wrapped line 4 and 5. Right now, the visual lines 1-3 are 
just empty, but still take place. Maybe it makes sense to omit fully empty 
lines completely?
  
  Can you reproduce this? I know this is tricky, so I'd be fine with committing 
the code as is. Or are you willing to put more efforts into 1. and/or 2. ? :-)
  
  In any case, this is already very much appreciated.

REPOSITORY
  R39 KTextEditor

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

To: hoffmannrobert, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D26463: Port endl as \n or std::cout ... std::endl when we wrote directly on console or flush QTextStream when it's a qstring

[Dominik Haumann]

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

REPOSITORY
  R39 KTextEditor

BRANCH
  port_endl (branched from master)

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

To: mlaurent, dfaure, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D26424: [kdiroperator] Add method for accessing actions without KActionCollection

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> kdiroperator.h:428
> +/**
> + * an accessor to a collection of all available Actions. The actions
> + * are static, they will be there all the time.

Imho the API documentation should be improved (I am aware it was copied).

  Returns a map of all available actions. The action map remains unchanged 
throughout the entire lifetime of the diroperator instance.
  
  The key of the map can be used to obtain the respective action. The full list 
of available actions is as follows:
  
  - popupMenu: ...
  - up: ...

REPOSITORY
  R241 KIO

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

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

D26321: Expose show-line-count in the ConfigInterface

[Dominik Haumann]

dhaumann accepted this revision.
dhaumann added a comment.


  Ok with this... Thanks. A note " Since 5.6x would be nice".

REPOSITORY
  R39 KTextEditor

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

To: mludwig, #ktexteditor, cullmann, dhaumann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, cent, LeGast00n, 
szutmael, GB_2, domson, michaelh, ngraham, bruns, demsking, head7, kfunk, sars

Re: CMake config & target challenges on moving to KF5 namespace; dir structure & API dox (Re: Submitting Grantlee as a KF5 Framework)

[Dominik Haumann]

Hi,

Stephen Kelly  schrieb am So., 29. Dez. 2019, 15:03:

>
> On 28/12/2019 23:30, Friedrich W. H. Kossebau wrote:
> > Why are you proposing to do a step back instead to the old state, which
> > everyone including you considered not that satisfying?
>
> Because it's a temporary situation. We still have a way forward in KF6
> (which will open in a few months).
>
>
> Generally, getting Grantlee into KF5 now also establishes the wrong
> precedent. Grantlee should be split into two repos each with a tier 1
> library (KF6::TextDocument and KF6::TextTemplate). The two are
> independent and have nothing to do with each other aside from
> authorship. That seems to be something you were objecting to, so I want
> to make sure that's something addressed on its own. The two KF6
> libraries will then follow the KF6 naming conventions etc.
>

With my KTextEditor hat on: KF6:TextDocument implies somehow a link to
QTextDocument or KF6:TextEditor, which both is incorrect, right?

Before starting this work, let's clarify whether we can find a more unique
name (like KF6:GrantleeTextDocument).

Since I haven't used Grantlee yet, I sm likely not the best person to find
a better name ;)

Best regards
Dominik

>

D21563: Fix empty pages in print preview and lines printed twice

[Dominik Haumann]

dhaumann added a comment.


  @hoffmannrobert: by the way, looking at your phabricator activity, you should 
get a KDE contributor account, if you don't have one already.

REPOSITORY
  R39 KTextEditor

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

To: hoffmannrobert, #kate, #ktexteditor, dhaumann
Cc: cullmann, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, LeGast00n, 
GB_2, domson, michaelh, bruns, demsking, sars

D21563: Fix empty pages in print preview and lines printed twice

[Dominik Haumann]

dhaumann added a comment.


  @hoffmannrobert: Are you maybe also interested in looking into 
https://bugs.kde.org/show_bug.cgi?id=415570 ? It's again about printing, this 
time about the very last line that seems to be wrong...

REPOSITORY
  R39 KTextEditor

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

To: hoffmannrobert, #kate, #ktexteditor, dhaumann
Cc: cullmann, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, LeGast00n, 
GB_2, domson, michaelh, bruns, demsking, sars

Re: Exemptions to try KF "grow" vs. consistent experience (Re: Submitting Grantlee as a KF5 Framework)

[Dominik Haumann]

Hi,

Volker Krause  schrieb am Fr., 27. Dez. 2019, 11:04:

> On Thursday, 26 December 2019 19:25:09 CET Albert Astals Cid wrote:
> > El dimarts, 24 de desembre de 2019, a les 13:05:23 CET, Friedrich W. H.
> Kossebau va escriure:
> > > Am Montag, 23. Dezember 2019, 09:57:57 CET schrieb Volker Krause:
> > > > On Sunday, 22 December 2019 09:46:02 CET Dominik Haumann wrote:
> > > > > Hi all,
> > > > >
> > > > > in any case, maybe the discussed points should go to the KF6
> > > > > workboard?
> > > > > https://phabricator.kde.org/project/view/310/
> > > > >
> > > > > I indeed believe that consistency in the KF5 world is an important
> > > > > feature,
> > > > > so Friedrich does have a point here. Other framework additions had
> to
> > > > > adapt
> > > > > as well (what comes to my mind is renaming of KQuickCharts or
> > > > > KCalendarCore).
> > > >
> > > > There is one important difference between
> KCalendarCore/KQuickCharts/etc
> > > > and Grantlee/QKeychain/etc though. The former had no previous release
> > > > promising a public API and therefore only KDE-internal users. The
> > > > latter have such releases and guarantees, and a significant
> > > > KDE-external user base. That's what makes me consider a transitional
> > > > pragmatic exemption from certain conventions, if we assume it would
> > > > help to grow our external user base, and thus the pool of potential
> > > > contributors.
> > >
> > > Having to make exemptions shows a principal design flaw though: if the
> > > idea is to pull in more libraries into KF, incl. some with previous
> > > releases & thus existing userbase hoping on longer-term stable ABI, the
> > > same will also happen in the KF6 series. And even for the currently
> > > discussed two libs Grantlee & QKeychain it means at least 1 1/2 years &
> > > longer (assuming KF 6.0.0 is coming then, and see how long kdelibs4
> > > survived) for being just "exemptions". It's rather that the
> "exemptions"
> > > become part of the rules that way.
> > Which kind of exceptions are we speaking about?
> >
> > ABI stability? or?
>
> The opposite actually :) It's about keeping existing API/ABI of frameworks
> with a significant (KDE external) user base when joining KF5, until the
> next
> major release, even if that means making compromises regarding e.g. our
> naming
> conventions.
>

If you are confident that the usually necessary steps to become an
"aligned" framework are going to be done in the kf6 transition, then I
believe this is ok.

That's why I mentioned that these steps should be added to the kf6
workboard.

Let's say Grantlee will not make these steps, then it's as simple as moving
it out of the kf6 circle again.

As I understand, you see a nice opportunity to make the KDE Frameworks more
recognized. I'd say, go for it.

Best regards
Dominik

>

Re: CMake config & target challenges on moving to KF5 namespace; dir structure & API dox (Re: Submitting Grantlee as a KF5 Framework)

[Dominik Haumann]

Hi all,

in any case, maybe the discussed points should go to the KF6 workboard?
https://phabricator.kde.org/project/view/310/

I indeed believe that consistency in the KF5 world is an important feature,
so Friedrich does have a point here. Other framework additions had to adapt
as well (what comes to my mind is renaming of KQuickCharts or
KCalendarCore).

Whatever decision is made here, imho there should/must be the objective to
get it fixed for KF6.

Best regards
Dominik

Friedrich W. H. Kossebau  schrieb am So., 22. Dez. 2019,
00:55:

> Am Samstag, 21. Dezember 2019, 23:32:10 CET schrieb Stephen Kelly:
> > On 21/12/2019 19:12, Friedrich W. H. Kossebau wrote:
> > > Am Samstag, 21. Dezember 2019, 13:03:17 CET schrieb Stephen Kelly:
> > >> Great, Grantlee is now available at g...@git.kde.org:grantlee.git.
> > >>
> > >> I've pushed a few commits to make it depend on ECM etc.
> > >>
> > >> Once the review period is finished it can be part of KF5 releases.
> > >
> > > There are quite some things which yet need to be done for now:
> > > to be a true KF module there is a set of policies which needs to be
> met,
> > > see https://community.kde.org/Frameworks/Policies
> > >
> > > 1) Framework directory structure:
> > > moving stuff into src/, autotests/ & docs/
> > >
> > > 2) Framework documentation:
> > > current system needs adaption to both online (KApiDox) and
> > > offline (ECMAddQCH) systems
> >
> > Cool, I wonder if there's another multi-library framework for comparison?
>
> With ECMAddQCH, Sonnet & KNewStuff create separate QCH files for their
> multiple libs.
>
> With KApiDox, IIRC it has the assumption 1 module <=> 1 documentation unit
> (not involved there),
> Olivier (cc:ed) should be able to hint you what is possible.
>
> > > Another challenge would be moving into the KF5 namespace for the
> library
> > > artifacts (at least I would expect/recommend this to happen, for
> > > consistent
> > > user experience)
> > > a) include dirs below subdir KF5/
> > > b) CMake modules with KF5 prefix
> > > c) CMake imported target in KF5 namespace
> >
> > I don't support changing things like this in the KF5 timeframe.
>
> IMHO not sharing the namespace "KF5" spoils the story of KDE Frameworks,
> where
> the namespace gives consistent developer experience and product messaging.
>
> Having Grantlee being a special kid, with unregular CMake modules names
> and
> differently namespace imported CMake targets, makes things more complex.
>
> Being consistent is what we so like about Qt, and KF should not stay
> behind,
> no?
>
> Perhaps joining the "Release Service" (formerly known as "KDE
> Applications")
> is a better place then, it also contains a set of libraries already.
> That would serve the purpose of having releases happening regularly.
>
> Cheers
> Friedrich
>
>
>

D25870: SELinux: add "glblub" keyword and update permissions list

[Dominik Haumann]

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

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  update-selinux

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

To: nibags, dhaumann, cullmann, #framework_syntax_highlighting
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D25869: Python: improve numbers, add octals, binaries and "breakpoint" keyword

[Dominik Haumann]

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

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  python-numbers

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

To: nibags, #framework_syntax_highlighting, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Dominik Haumann]

dhaumann added a comment.


  In D23457#574846 , @ahmadsamir 
wrote:
  
  > Personally, I am not convinced replacing \s with [ \t], and deviating from 
PCRE default behaviour is a good idea in this case.
  
  
  That is not to be discussed, and Christoph tried to explain this already 
before.
  
  In short: If these issues don't get fixed to be compatible with previous 
behavior, this patch will not get merged. I'd be very much in favor of having 
the compatibility, since QRegularExpression is the way forward, but adding 
regressions is a big no-go and not up for discussion.
  
  We are maintaining Kate for close to 20 years now. We'd like to ask you to 
give us some trust in our decision.

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D23457: Port regex search to QRegularExpression

[Dominik Haumann]

dhaumann added a comment.


  Was anything of the previously commented issues addressed?

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D25767: KAutoSaveFile: add a unit test to check max. filename length

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> dfaure wrote in kautosavefiletest.cpp:98
> Let's hope none of the supported OSes have a smaller PATH_MAX :)
> FreeBSD, Windows, macOS, who knows what limits they have.

What I wonder is: PATH_MAX etc are #defines, right? Since on Windows there is a 
registry flag to enable much longer paths/file names. My intention is not to 
block this series of patches, but we might end up with restrictions in our code 
that do not exist on the filesystem at hand.

REPOSITORY
  R244 KCoreAddons

BRANCH
  l-kautosave-unittest (branched from master)

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

To: ahmadsamir, #frameworks, mpyne, dfaure
Cc: dhaumann, dfaure, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D25755: Replace iterators with range-based for

[Dominik Haumann]

dhaumann added a comment.


  >>> Do we dislike iterators now?
  >> 
  >> We don't, and they still make sense for when you need the `key`, but range 
for is just much nier to look at :)
  > 
  > I'm fine with that statement. But are we going to be reviewing changing all 
the KDE code from iterators to range for? Feels like an overkill to me.
  
  It's much more than just the fact "it's nicer to look at". It' i) more 
compact, but more importantly, it's ii) simpler to reason about. When having 
iterator-based loops you cannot immediately say "we iterate over all items". It 
could be that the iterator is not increased, or it is increased in the body 
scope multiple times. Contrary, with range-based for loops you know what you 
*always* iterate over all items (except if there is a shortcut break/return 
statement).
  
  The only nitpick is: In the Qt world this is sometimes tricky when using 
range-based for loops over non-const Qt containers, which leads to a detach. So 
review is typically necessary.
  
  PS: Yes, it should be `const QString ` and `const QString `, 
i.e. references and not copies.

REPOSITORY
  R297 KDED

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

To: nicolasfella, #frameworks
Cc: dhaumann, dfaure, ahmadsamir, broulik, aacid, apol, kde-frameworks-devel, 
LeGast00n, GB_2, michaelh, ngraham, bruns

D25770: Several enhancements to gitolite syntax definition.

[Dominik Haumann]

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


  Looks good. But does the highlighting work for `RW+CD`? I am wondering 
whether `+` needs to be added to the weakDeliminator list?

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  gitolite-syntax (branched from master)

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

To: zrax, dhaumann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars

D25276: reStructuredText: Fix inline literals highlighting preceding characters

[Dominik Haumann]

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


  Ok

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  rst-inline-literal-prefix

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

To: adrianchavesfernandez, mosra, dhaumann
Cc: nibags, dhaumann, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars

D25698: New class KApplicationTrader, to replace KMimeTypeTrader and KServiceTypeTrader

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> kapplicationtrader.h:33
> + *
> + * Example: say that you want to the list of all applications that can 
> handle PNG images.
> + * The code would look like:

Typo: you want to _get_...

REPOSITORY
  R309 KService

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

To: dfaure, broulik, mart, vkrause, nicolasfella
Cc: dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25493: EBN kmoretools cleanup transport protocol

[Dominik Haumann]

dhaumann added a comment.


  @jhayes Do you have commit access, or shall I commit this for you?

REPOSITORY
  R304 KNewStuff

BRANCH
  ebn-cleanup (branched from master)

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

To: jhayes, dhaumann
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23457: Port regex search to QRegularExpression

[Dominik Haumann]

dhaumann added a comment.


  Any news here?

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, dhaumann, cullmann
Cc: kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24982: Small improvements in some XML files

[Dominik Haumann]

dhaumann added a comment.


  I think we should decide what to do with this patch, as over time it will get 
merge conflicts.
  
  In general, I hoped we can have some sort of auto detection to disable 
captures in a smart way.
  
  If this is not possible, maybe we should accept this patch?
  
  Did you measure speed improvements?

REPOSITORY
  R216 Syntax Highlighting

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

To: nibags, #framework_syntax_highlighting, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D25508: Make KF6 cleanup work visible

[Dominik Haumann]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:c8fa3ab044cb: Make KF6 cleanup work visible (authored by 
dhaumann).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25508?vs=70264=70266

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

AFFECTED FILES
  src/kmoretools/kmoretoolsmenufactory.h

To: dhaumann, gregormi, ngraham
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25508: Make KF6 cleanup work visible

[Dominik Haumann]

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

REVISION SUMMARY
  This work is simply about removing an unused private member
  variable that is in an exported class.

TEST PLAN
  make && make test

REPOSITORY
  R304 KNewStuff

BRANCH
  work-add-kf6-note-kmoretools

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

AFFECTED FILES
  src/kmoretools/kmoretoolsmenufactory.h

To: dhaumann, gregormi
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25503: JavaScript: move keywords from TypeScript and other improvements

[Dominik Haumann]

dhaumann added a comment.


  I think this is ok.
  
  I recognize only now that a much better syntax would have been
  
variable_property
  
  but that is off-topic :)
  
  If the kateversions are correct, then I'm fine with this.

REPOSITORY
  R216 Syntax Highlighting

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

To: nibags, cullmann, dhaumann, #framework_syntax_highlighting
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D25493: EBN kmoretools cleanup transport protocol

[Dominik Haumann]

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

REPOSITORY
  R304 KNewStuff

BRANCH
  ebn-cleanup (branched from master)

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

To: jhayes, dhaumann
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D17595: Upstream Dolphin's file rename dialog

[Dominik Haumann]

dhaumann added a comment.


  In D17595#542142 , @meven wrote:
  
  > I received the agreements of all contributors including the original code 
author, to relicense this code and hence upstream it.
  
  
  Playing the bad guy here: where are these agreements? On a public mailing 
list, or in your private inbox? If on a public mailing list, you could 
reference these mails on our mail archives in your commit message.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham

KF6 Blogs

[Dominik Haumann]

Hi everyone,

could you who currently are at the KF6 sprint blog a bit more?

Even if it's just about small details like "app X does not depend on Y
anymore and this is how we got rid of the dependency" already is valuable
information, since it helps other contributors to not introduce such code
anymore.

The KDE Frameworks are our foundations for pretty much everything we do. It
deserves more press coverage, including maybe a final dot story.

Keep it up :-)
Dominik

D25485: Remove KIconThemes use

[Dominik Haumann]

dhaumann added a comment.


  Makes sense, thanks.

REPOSITORY
  R39 KTextEditor

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

To: broulik, #frameworks, cullmann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars

D25465: [DropJob] Minor grammar fix in API docs

[Dominik Haumann]

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


  Lgtm.

REPOSITORY
  R241 KIO

BRANCH
  l-grammar (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, apol, dhaumann
Cc: dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25276: reStructuredText: Fix inline literals highlighting preceding characters

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> rest.xml:14
>-->
>   mimetype="text/x-rst" version="5" kateversion="2.4" casesensitive="1">
>  

Please increase the version from 5 to 6. Every time a highlighting file is 
changed this needs to be done.

REPOSITORY
  R216 Syntax Highlighting

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

To: adrianchavesfernandez, mosra
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars

D25328: Always rehiglhight() after definition was changed

[Dominik Haumann]

dhaumann added a comment.


  Can't you call `rehighlight()` yourself after calling `setDefinition()`?
  
  Btw, tanks for looking into this!

REPOSITORY
  R216 Syntax Highlighting

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

To: meven, kossebau, cullmann, vkrause
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars

D24568: Provide clang-format target with a KDE Frameworks style file

[Dominik Haumann]

dhaumann added a comment.


  You can force the current clang format to keep the multi-line if as follows:
  
if (roles.isEmpty() // comment
|| roles.contains(Notifications::UpdatedRole)
|| roles.contains(Notifications::ExpiredRole)
  
  The comment prevents clang to join the lines. I understand it's not what you 
want, though.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, ochurlaud, nalvarez, kossebau, aacid, davidedmundson, 
dhaumann, apol, ognarb, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D25054: Add string escape characters to PowerShell syntax.

[Dominik Haumann]

dhaumann added a comment.


  Looks good to me.
  
  Btw, you posted several patches over the last two years. You could also apply 
for a KDE contributor account and push yourself, if you want.

REPOSITORY
  R216 Syntax Highlighting

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

To: zrax, dhaumann, cullmann
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, 
domson, michaelh, ngraham, bruns, demsking, sars

D25054: Add string escape characters to PowerShell syntax.

[Dominik Haumann]

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


  Please commit.

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  ps-escape (branched from master)

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

To: zrax, dhaumann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars

D24982: Small improvements in some XML files

[Dominik Haumann]

dhaumann added a comment.


  Hm right... too bad. I was hoping to find an automated way to detect this. 
Since relying on the user to optimize the RegExps will always be suboptimal. 
@cullmann Do you have any ideas?

REPOSITORY
  R216 Syntax Highlighting

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

To: nibags, #framework_syntax_highlighting, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24983: KateModeMenuList: improve word wrap

[Dominik Haumann]

dhaumann added a comment.


  Good, fine with me.
  
  Related: Maybe the translation should also be shorter, but the general 
possible problem persists. Please go ahead.

REPOSITORY
  R39 KTextEditor

BRANCH
  improve-word-wrap

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

To: nibags, #ktexteditor, cullmann, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24982: Small improvements in some XML files

[Dominik Haumann]

dhaumann added a comment.


  > [...]
  >  One option would be to add a **capture** or **dontCapture** attribute to 
enable or disable captures for RegExpr rules. Also, captures could be enabled 
or disabled in all RegExpr rules using the  group, adding an 
element for that.
  
  But don't we have this already with the `dynamic` flag? Or am I mixing things?

REPOSITORY
  R216 Syntax Highlighting

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

To: nibags, #framework_syntax_highlighting, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

Re: KSyntaxHighlighting: My changes to data files don’t seem to affect test outputs

[Dominik Haumann]

What does `ldd build/bin/syntax/testhighlighter_test` tell you?
What, if you first source build/prefix.sh?

Likely the wrong lib is used.

Greetings
Dominik


Adrian Chaves  schrieb am So., 27. Okt. 2019, 17:49:

> I emptied data/syntax/rest.xml (made it a valid syntax file that would
> mark all text as normal).
>
> Then I run:
>
>  rm -rf build && mkdir build && cd build
>  cmake .. && make -j8 && make test
>
> But the contents of build/autotests/html.output/highlight.rst.html are
> still those that would be generated if data/syntax/rest.xml had not been
> changed.
>

D24983: KateModeMenuList: improve word wrap

[Dominik Haumann]

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


  It's hard to review all the pixel changes with no screenshots. It's even 
unclear to me what problem exactly is fixed at hand. Given it's all your code, 
I trust you know what you are doing...

REPOSITORY
  R39 KTextEditor

BRANCH
  improve-word-wrap

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

To: nibags, #ktexteditor, cullmann, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24982: Small improvements in some XML files

[Dominik Haumann]

dhaumann added a comment.


  I wonder if the `?:` optimizations make sense. QRegularExpression has the 
option `QRegularExpression::DontCaptureOption` to not capture anything. Looking 
into our code we have:
  
561 bool RegExpr::doLoad(QXmlStreamReader& reader)
562 {
563 
m_regexp.setPattern(reader.attributes().value(QStringLiteral("String")).toString());
   // here we set the pattern -> OK
564
565 const auto isMinimal = 
Xml::attrToBool(reader.attributes().value(QStringLiteral("minimal")));
566 const auto isCaseInsensitive = 
Xml::attrToBool(reader.attributes().value(QStringLiteral("insensitive")));
567 m_regexp.setPatternOptions( 
   // if (m_dynamic == false), we could add 
the
568(isMinimal ? QRegularExpression::InvertedGreedinessOption : 
QRegularExpression::NoPatternOption) |  // flag 
QRegularExpression::DontCaptureOption
569(isCaseInsensitive ? QRegularExpression::CaseInsensitiveOption : 
QRegularExpression::NoPatternOption));
570
571// optimize the pattern for the non-dynamic case, we use them OFTEN
572m_dynamic = 
Xml::attrToBool(reader.attributes().value(QStringLiteral("dynamic")));
573if (!m_dynamic) {
574m_regexp.optimize();
575}
576// [...]
  
  In other words: The current patch adds many `?:` which also makes the RegExps 
harder to read. So: Do we really get a performance gain here? Wouldn't it be 
possible to get an even better performance gain by using the flag 
`DontCaptureOption`?
  
  Currently, I am not yet convinced, can you give this a try? :-)

REPOSITORY
  R216 Syntax Highlighting

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

To: nibags, #framework_syntax_highlighting, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24981: Modelines: fix end of comment

[Dominik Haumann]

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

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  fix-end-modelines

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

To: nibags, #framework_syntax_highlighting, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24939: Meson: more built-in functions and add built-in member functions

[Dominik Haumann]

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


  Looks good to me, thanks!

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  meson (branched from master)

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

To: jpoelen, #framework_syntax_highlighting, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24620: Windows MSVC compile fix

[Dominik Haumann]

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


  Imo we should simply try: We have another two weeks for testing.

REPOSITORY
  R159 KActivities Statistics

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

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

D24568: Provide clang-format target with a common KDE style file

[Dominik Haumann]

dhaumann added a comment.


  I'm all for it. This would unify how we can reformat any KDE module, which is 
very much desirable.
  
  Being able to just reformat a patch sounds interesting, too, but can be added 
later still.
  
  Don't let the perfect be the enemy of the good...
  
  The example usage should go to the location that Carl pointed out.
  
  +1, can we have another iteration?

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann
Cc: dhaumann, apol, ognarb, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
GB_2, bencreasy, michaelh, ngraham, bruns

D24578: Meson: Add a comment section for comment/uncomment with Kate

[Dominik Haumann]

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


  Please increase the version number and commit.

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  meson (branched from master)

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

To: jpoelen, #framework_syntax_highlighting, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24443: Add a plugin system

[Dominik Haumann]

dhaumann added a comment.


  I'd go for a QVector for now. Arguing with Qt6 doesn't sound convincing to 
me, since Qt6 will take another>=1 year(s). So why try this experiment in 
public API now? :)

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

To: nicolasfella, #frameworks, #plasma
Cc: dhaumann

D24415: Add standard icons to support to all entries in QDialogButtonBox

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> kstyle.cpp:422
>  }
> +#if QT_VERSION >= 0x050E00 // Check if Qt version >= 5.14
> +case QStyle::SP_DialogYesToAllButton:

Instead of 0x050E00 you could use the macro QT_VERSION_CHECK(5, 14, 0). This 
would be a bit more self explanatory.

REPOSITORY
  R252 Framework Integration

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

To: GB_2, #frameworks, #vdg, davidedmundson
Cc: dhaumann, davidedmundson, #vdg, kde-frameworks-devel, #frameworks, 
LeGast00n, GB_2, michaelh, ngraham, bruns

D24403: Small performance improvements suggested by clang-tidy

[Dominik Haumann]

dhaumann accepted this revision.
dhaumann added a comment.


  I also think it's fine. A next patch could convert the QRegExp to a 
QRegularExpression.

REPOSITORY
  R39 KTextEditor

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

To: aacid, cullmann, dhaumann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, 
domson, michaelh, ngraham, bruns, demsking, sars

D24404: Small performance improvements suggested by clang-tidy

[Dominik Haumann]

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

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  master

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

To: aacid, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24354: Mustache/Handlebars: minor fixes

[Dominik Haumann]

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

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  fix-delimiters-mustache

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

To: nibags, #framework_syntax_highlighting, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24399: Pass QDir by const & instead of copy

[Dominik Haumann]

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

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

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

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

D13541: Port solid from Qt5::Widgets to Qt5::Gui

[Dominik Haumann]

dhaumann added a comment.


  @graesslin pong? One year passed.

REPOSITORY
  R245 Solid

BRANCH
  gui-instead-of-widgets

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

To: graesslin, #frameworks, dhaumann, apol, broulik
Cc: dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24378: WordDetect rule: detect delimiters at the inner edge of the string

[Dominik Haumann]

dhaumann added a comment.


  I think it's fine as is. The docbook says:
  
Detect an exact string but additionally require word boundaries
such as a dot '.' or a whitespace on the beginning
and the end of the word. Think of \bstring\b
in terms of a regular expression, but it is faster than the rule 
RegExpr.
  
  Imo `\bstring\b` implies that if a string 
itself starts/ends with a \b character, then this should match as well. And 
given our unit tests do not show any changes, I think we are good to go.
  
  Please commit.

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  fix-worddetect

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

To: nibags, #framework_syntax_highlighting, dhaumann, cullmann, vkrause, jpoelen
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24378: WordDetect rule: detect delimiters at the inner edge of the string

[Dominik Haumann]

dhaumann added a comment.


  This looks good to me and as mentioned in D24354 
 WordDetect is better than RegExpr.
  
  +1, but I'd like another review by @cullmann or @vkrause.

REPOSITORY
  R216 Syntax Highlighting

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

To: nibags, #framework_syntax_highlighting, dhaumann, cullmann, vkrause, jpoelen
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24326: Add syntax highlighting for RenPy (.rpy)

[Dominik Haumann]

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

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  renpy (branched from master)

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

To: jpoelen, #framework_syntax_highlighting, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24354: Mustache/Handlebars: fix delimiters in HTML tags

[Dominik Haumann]

dhaumann added a comment.


  Thanks a lot!

REPOSITORY
  R216 Syntax Highlighting

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

To: nibags, #framework_syntax_highlighting, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24354: Mustache/Handlebars: fix delimiters in HTML tags

[Dominik Haumann]

dhaumann added a comment.


  Hm, would it make sense to change this? I.e. if the word itself has a word 
boundary, also use this?

REPOSITORY
  R216 Syntax Highlighting

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

To: nibags, #framework_syntax_highlighting, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24354: Mustache/Handlebars: fix delimiters in HTML tags

[Dominik Haumann]

dhaumann added a comment.


  Would it also be an option to fix WordDetect? I always thought WordDetect 
ignores the string contents, it should only check on the boundaries left and 
right

REPOSITORY
  R216 Syntax Highlighting

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

To: nibags, #framework_syntax_highlighting, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D24262: Modernize code: use range-based loops & algorithms in more places

[Dominik Haumann]

dhaumann added a comment.


  In D24262#539317 , @dfaure wrote:
  
  > It's actually quite clear in my head, because I imagine the generated 
class. A captured variable in a lambda becomes a member variable. If it's a 
capture by value (which is what happens with [library]), it's a "plain value" 
member.
  > So
  
  
  David's explanation is much more correct: Behind the scenes you have this 
Closure trick with a functor object, not really a free function what I was 
claiming ;)

REPOSITORY
  R306 KParts

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

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

D24262: Modernize code: use range-based loops & algorithms in more places

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> kossebau wrote in plugin.cpp:196
> Confirmed by experiments. Still not yet found a document where explicitly it 
> is mentioned that the copy constructor will be invoked to generate a copy of 
> the object for any captured variables only being of type reference, so if 
> anyone can point out one which reads this clearly to me, happy to get a 
> reference to, so I can try to do a copy of that referenced document into my 
> brain lambda :)

Understanding captures becomes simple as soon as you follow what the compiler 
essentially does for you with the lambda:
The lambda function does not capture `[this]`, so behind the scenes the 
compiler will create for you a free function (with an internal unique function 
name). This free function will have a local const variable "library". If you 
write [library] than this local variable will simply be a copy (think of the 
definition `const QString library = library_from_outer_scope;`. When you write 
`[]` instead, you will get a `const QString  = 
library_from_outer_scope;`. Does that clarify when to use `&` or not? :-)

PS: If you capture `[this]`, then the compiler will not a create a free 
function, instead the compiler will create a member function for you (again 
with internal unique name ).

Looking at lambda functions from this perspective makes this rather simple.

REPOSITORY
  R306 KParts

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

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

D24262: Modernize code: use range-based loops & algorithms in more places

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> plugin.cpp:196
>  
> -QObjectList::ConstIterator it = plugins.begin();
> -for (; it != plugins.end(); ++it) {
> -Plugin *plugin = qobject_cast(*it);
> -if (plugin && plugin->d->m_library == library) {
> -return true;
> -}
> -}
> -return false;
> +return std::any_of(plugins.begin(), plugins.end(), [library](QObject* p) 
> {
> +Plugin *plugin = qobject_cast(p);

cbegin() + cend()
Catch library by reference? [& library]

> statusbarextension.cpp:139
>  if (d->m_activated) {
> -QList::iterator it = d->m_statusBarItems.begin();
> -for (; it != d->m_statusBarItems.end(); ++it) {
> -(*it).ensureItemShown(sb);
> +for (auto& item : d->m_statusBarItems) {
> +item.ensureItemShown(sb);

qAsConst?

> statusbarextension.cpp:143
>  } else {
> -QList::iterator it = d->m_statusBarItems.begin();
> -for (; it != d->m_statusBarItems.end(); ++it) {
> -(*it).ensureItemHidden(sb);
> +for (auto& item : d->m_statusBarItems) {
> +item.ensureItemHidden(sb);

Same here?

REPOSITORY
  R306 KParts

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

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