dhaumann added a comment.
The patch looks already much better. I have added comments below.
Btw, did you copy some code from another project? If so, we need to be
careful, since KTextEditor is LGPLv+2.
INLINE COMMENTS
> CMakeLists.txt:54
> +# EditorConfig support
> +# TODO: find oldest
dhaumann added a reviewer: alexmerry.
REPOSITORY
R240 Extra CMake Modules
REVISION DETAIL
https://phabricator.kde.org/D4589
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: gszymaszek, #build_system, #frameworks, alexmerry
Cc: dhaumann
dhaumann added a comment.
Correct is that we want to write
find_package(editorconfig "0.12.0)
i.e., with a version. This is missing in this find module, see the LibGit2
find module. Could you extend this?
REPOSITORY
R240 Extra CMake Modules
REVISION DETAIL
brauch added a comment.
Sorry, I wanted to write a reply but failed. My idea was simply to have the
algorithm always aim to make parentheses balanced when closing one.
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D4234
EMAIL PREFERENCES
This revision was automatically updated to reflect the committed changes.
Closed by commit R236:866fdf0802b5: Fix KEditListWidget losing the focus on
click of buttons (authored by kossebau).
REPOSITORY
R236 KWidgetsAddons
CHANGES SINCE LAST UPDATE
gszymaszek created this revision.
gszymaszek added reviewers: Build System, Frameworks.
gszymaszek set the repository for this revision to R240 Extra CMake Modules.
gszymaszek added a project: Frameworks.
Restricted Application added a project: Build System.
REVISION SUMMARY
EditorConfig
gszymaszek updated this revision to Diff 11262.
gszymaszek added a comment.
Moved EditorConfig-related logic into a separate class and made it optional
(no longer a KTextEditor dependency).
REPOSITORY
R39 KTextEditor
CHANGES SINCE LAST UPDATE
anthonyfieroni added a comment.
> Also: how do you know whether your change will still work in Qt 5.7?
I noticed same downside there, if someone still has Qt 5.7 can test it.
Tested Qt 5.6 on Kubuntu backport -> works.
REPOSITORY
R242 Plasma Framework (Library)
REVISION DETAIL
shaheed added inline comments.
INLINE COMMENTS
> skelly wrote in sip_generator.py:172
> It was possible to handle exports without looking for the text EXPORT in the
> MACRO NAME. Why is deprecated different?
Because the expansion of the attribute in this case contains not a string, but
a
flherne closed this revision.
flherne added a comment.
https://cgit.kde.org/ktexteditor.git/commit/?id=b4006363eaf1d9d26f037bcdbc5f1a283b9d109e
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D4421
EMAIL PREFERENCES
shaheed added inline comments.
INLINE COMMENTS
> skelly wrote in rules_engine.py:494
> Does the docs match the code if you add this here? Why is this in this commit?
Yes. Why not?
REPOSITORY
R240 Extra CMake Modules
REVISION DETAIL
https://phabricator.kde.org/D4509
EMAIL PREFERENCES
dhaumann accepted this revision.
dhaumann added a reviewer: dhaumann.
dhaumann added a comment.
This is fine, please commit.
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D4421
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To:
shaheed added a comment.
I think the scope of the changes in this commit are perfectly reasonable. I
don't think it is unreasonable to group them either.
REPOSITORY
R240 Extra CMake Modules
REVISION DETAIL
https://phabricator.kde.org/D4509
EMAIL PREFERENCES
shaheed added inline comments.
INLINE COMMENTS
> skelly wrote in rules_engine.py:58
> I don't think this should be here.
I disagree. Having this in the code suppresses the false positives which are
caused by the "_" used by gettext.
> skelly wrote in rules_engine.py:704
> I don't think the
dhaumann added a comment.
What's the current state of this?
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D4234
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: cactus, #ktexteditor
Cc: dhaumann, brauch, cullmann,
dhaumann added a comment.
If you want, you can also just copy or duplicate these functions, and we
later look how to merge them again in a sane way.
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D4537
EMAIL PREFERENCES
skelly added a comment.
In https://phabricator.kde.org/D4509#85725, @shaheed wrote:
> The PEP-8 changes are some blank line changes.
There is exactly one blank line insertion, and it makes this TypedefRuleDb
documentation inconsistent with, say, the VariableRuleDb, which doesn't
shaheed added a comment.
The PEP-8 changes are some blank line changes.
REPOSITORY
R240 Extra CMake Modules
REVISION DETAIL
https://phabricator.kde.org/D4509
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: shaheed, #build_system, #frameworks, skelly
graesslin added a comment.
> Looks like Qt 5.8 has a grabber bug
Then Qt should fix and not we workaround it.
Also: how do you know whether your change will still work in Qt 5.7?
REPOSITORY
R242 Plasma Framework (Library)
REVISION DETAIL
https://phabricator.kde.org/D4587
EMAIL
skelly added inline comments.
INLINE COMMENTS
> sip_generator.py:172
> """
> +if member.kind == CursorKind.UNEXPOSED_ATTR and
> text.find("_DEPRECATED") != -1:
> +sip["annotations"].add("Deprecated")
It was possible to handle exports without looking for the text
skelly added inline comments.
INLINE COMMENTS
> rules_engine.py:58
> +# Keep PyCharm happy.
> +_ = _
> +
I don't think this should be here.
> rules_engine.py:494
> nameThe name of the
> typedef.
> +
skelly added a comment.
Is anything here actually a PEP 8 fix?
REPOSITORY
R240 Extra CMake Modules
REVISION DETAIL
https://phabricator.kde.org/D4509
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: shaheed, #build_system, #frameworks, skelly
Cc:
anthonyfieroni created this revision.
anthonyfieroni added a reviewer: Plasma.
anthonyfieroni added subscribers: mart, davidedmundson.
anthonyfieroni set the repository for this revision to R242 Plasma Framework
(Library).
Restricted Application added projects: Plasma, Frameworks.
Restricted
thomasp reopened this revision.
thomasp added a comment.
This revision is now accepted and ready to land.
Alternative tokens [lex.digraph] are standard c++ and have been since it's
was standardized (and I don't know of any plan to deprecate them). They are a
accessibility feature, since not
broulik added a comment.
`QCoreApplication::desktopFileName()` is the full desktop file name without
the path, ie. as far as I can tell with the `.desktop` suffix, the Gnome spec
explicitly says it should be sent without it, though.
REPOSITORY
R289 KNotifications
REVISION DETAIL
gszymaszek added a comment.
In the new `EditorConfig` class I need to use `DocumentPrivate`’s
`checkBoolValue` and `checkIntValue` methods, but they’re declared private. I
think it should be OK to make them public as they’re static anyway, but I’d
like to hear what’s your opinion. Setting
dhaumann added a comment.
Yes, please create a separate review request for the extra-cmake-modules
repository. There, you will also get a good review by developers who know cmake
very well (which I do not), so in the end we will also have a very god
Findeditorconfig cmake module. Thanks!
This revision was automatically updated to reflect the committed changes.
Closed by commit R39:268af3d2b7ea: Use C++11 log2() instead of log() / log(2)
(authored by dhaumann).
REPOSITORY
R39 KTextEditor
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D4300?vs=10602=11255
REVISION
gszymaszek added a comment.
Thanks for your comments. I’ve managed to make editorconfig optional using
the mentioned `FindEditorConfig.cmake` module (I had to change its name to
`Findeditorconfig.cmake`). Should I create a new diff in ECM repository to add
this file?
Now I’m going to
dhaumann added a comment.
A good patch, thanks!
REPOSITORY
R216 Syntax Highlighting
REVISION DETAIL
https://phabricator.kde.org/D4296
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: gszymaszek, #framework_syntax_hightlighting
Cc: dhaumann,
This revision was automatically updated to reflect the committed changes.
Closed by commit R216:72ca652acd16: less highlighting: Fix single line comments
starting new regions (authored by dhaumann).
REPOSITORY
R216 Syntax Highlighting
CHANGES SINCE LAST UPDATE
dhaumann added a comment.
By the way: The find module for libgit2 is here:
https://github.com/KDE/extra-cmake-modules/blob/master/find-modules/FindLibGit2.cmake
Maybe you can add one (if none exists already) for the EditorConfig lib?
REPOSITORY
R39 KTextEditor
REVISION DETAIL
dhaumann added a comment.
Cool, I have some comments, though :-)
1. Optional Dependency
As you yourself note, please make this an optional dependency: Best is if
we could use find_package(editorconfig) or so to make sure it is consistent how
we typically also add dependencies.
GENERAL INFO
BUILD SUCCESS
Build URL:
https://build.kde.org/job/ktextwidgets%20master%20stable-kf5-qt5/PLATFORM=Linux,compiler=gcc/154/
Project: PLATFORM=Linux,compiler=gcc
Date of build: Sun, 12 Feb 2017 14:31:07 +
Build duration: 5 min 0 sec
CHANGE SET
No changes
JUNIT RESULTS
GENERAL INFO
BUILD SUCCESS
Build URL:
https://build.kde.org/job/ktextwidgets%20master%20stable-kf5-qt5/PLATFORM=Linux,compiler=gcc/154/
Project: PLATFORM=Linux,compiler=gcc
Date of build: Sun, 12 Feb 2017 14:31:07 +
Build duration: 5 min 0 sec
CHANGE SET
No changes
JUNIT RESULTS
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129943/#review102511
---
Ship it!
Thanks, I committed the patch with minor some
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129943/
---
(Updated Feb. 12, 2017, 3:16 p.m.)
Status
--
This change has been
mwolff added inline comments.
INLINE COMMENTS
> kdirwatch.cpp:371
> // files in WatchFiles mode with inotify.
> if (isDir) {
> addEntry(client->instance, tpath,
> nullptr, isDir,
future patch
dfaure accepted this revision.
dfaure added a reviewer: dfaure.
This revision is now accepted and ready to land.
REPOSITORY
R236 KWidgetsAddons
BRANCH
makeKListEditWidgetNotLoseFocus
REVISION DETAIL
https://phabricator.kde.org/D4392
EMAIL PREFERENCES
dfaure created this revision.
dfaure added reviewers: aacid, mpyne.
dfaure added a subscriber: Frameworks.
Restricted Application added a project: Frameworks.
REVISION SUMMARY
Using std::vector rather than QVector in order to be able to use emplace_back
to avoid copying.
TEST PLAN
ctest
dfaure accepted this revision.
dfaure added a reviewer: dfaure.
This revision is now accepted and ready to land.
REPOSITORY
R266 Breeze Icons
REVISION DETAIL
https://phabricator.kde.org/D4448
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: svuorela,
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129943/
---
Review request for KDE Frameworks, Christoph Feck and Eike Hein.
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129663/#review102508
---
"Qt doesn't do this" = Qt doesn't strip '&' from action
43 matches
Mail list logo
