D5788: Add syntax highlighting for YANG data modeling language
aacid added a comment. Any reason not pushed? REPOSITORY R216 Syntax Highlighting BRANCH master REVISION DETAIL https://phabricator.kde.org/D5788 To: nalvarez, #kate, jkt, dhaumann Cc: aacid, dhaumann, jkt, #frameworks
D1231: Add Remote Access interface to KWayland
Kanedias updated this revision to Diff 15553. Kanedias added a comment. Fixed KWAYLAND -> Kwayland in CmakeLists.txt REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1231?vs=15125=15553 REVISION DETAIL https://phabricator.kde.org/D1231 AFFECTED FILES autotests/client/CMakeLists.txt autotests/client/test_remote_access.cpp src/client/CMakeLists.txt src/client/fakeinput.cpp src/client/fakeinput.h src/client/protocols/fake-input.xml src/client/protocols/remote-access.xml src/client/registry.cpp src/client/registry.h src/client/remote_access.cpp src/client/remote_access.h src/server/CMakeLists.txt src/server/display.cpp src/server/display.h src/server/fakeinput_interface.cpp src/server/fakeinput_interface.h src/server/remote_access_interface.cpp src/server/remote_access_interface.h src/server/remote_access_interface_p.h src/tools/mapping.txt src/tools/testserver/testserver.cpp To: Kanedias, graesslin, davidedmundson Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas
D1231: Add Remote Access interface to KWayland
Kanedias added a comment. @davidedmundson , do we really have nativeResourceForScreen call? AFAIK KWin uses its own QPA, which implements only bits of functionality. We need to have nativeResourceForScreen to be able to pass wl_output, should I patch QPA also, or is there better way? REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas
D6250: Expand documentation of Persistence attribute
davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.
Polkit-1 has the a concept of persistence, under a different name. A policy
can be set to:
- auth_admin
- auth_admin_keep
policy-gen-polkit1.cpp chooses between these using the persistence flag
if (!action.persistence.isEmpty() && policy != QLatin1String("yes") &&
policy !=
QLatin1String("no")) {
policy += QLatin1String("_keep");
}
Which is saying if persistence is set to anything and the policy is either
"auth" or "auth_self" append "_keep"
Granted that doesn't match the current docs, but WRT
https://phabricator.kde.org/D6198 we can delete that persistence attribute and
it won't linger.
REPOSITORY
R283 KAuth
REVISION DETAIL
https://phabricator.kde.org/D6250
To: elvisangelaccio, #frameworks, davidedmundson
Cc: davidedmundson
D6226: KKeySequenceWidget: make it possible to record Ctrl+Num+1 as a shortcut.
dfaure added inline comments. INLINE COMMENTS > davidedmundson wrote in kkeysequencewidget.cpp:778 > Should this section be using ShiftModifier? Qt::SHIFT has been masked out qglobal.h: SHIFT = Qt::ShiftModifier so it's the same (in case your sentence was making a difference about the two). And Shift hasn't been masked out, it's been masked in :) REPOSITORY R263 KXmlGui REVISION DETAIL https://phabricator.kde.org/D6226 To: dfaure, aacid Cc: davidedmundson, apol, #frameworks
D6198: Add KAuth support to delete operation
elvisangelaccio added inline comments. INLINE COMMENTS > davidedmundson wrote in file.actions:5 > We don't want session persistence > > It would mean if you prompt the user to delete a file, any rogue plugin in > Dolphin can now remove any file without prompts. > We don't want session persistence KAuth is actually misleading here, see https://phabricator.kde.org/D6250 (TL;DR on polkit-1 systems, there is no such thing as session persistence) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6198 To: chinmoyr, elvisangelaccio, #frameworks Cc: davidedmundson
D6198: Add KAuth support to delete operation
davidedmundson added inline comments.
INLINE COMMENTS
> file.actions:5
> +Policy=auth_admin
> +Persistence=session
We don't want session persistence
It would mean if you prompt the user to delete a file, any rogue plugin in
Dolphin can now remove any file without prompts.
> helper.cpp:46
> +if (errno == EACCES || errno == EPERM) {
> +QVariantMap errorData;
> +errorData.insert(QStringLiteral("errormessage"), i18n("Access
> Denied!"));
Rather than replying with Success when you fail, with your own structure use
setError (not errorCode) and setErrorDescription
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D6198
To: chinmoyr, elvisangelaccio, #frameworks
Cc: davidedmundson
D6226: KKeySequenceWidget: make it possible to record Ctrl+Num+1 as a shortcut.
davidedmundson added inline comments.
INLINE COMMENTS
> kkeysequencewidget.cpp:778
> if (keyQt) {
> if ((keyQt == Qt::Key_Backtab) && (d->modifierKeys & Qt::SHIFT))
> {
> keyQt = Qt::Key_Tab | d->modifierKeys;
Should this section be using ShiftModifier? Qt::SHIFT has been masked out
REPOSITORY
R263 KXmlGui
REVISION DETAIL
https://phabricator.kde.org/D6226
To: dfaure, aacid
Cc: davidedmundson, apol, #frameworks
D6234: KGlobalAccel: port to KKeyServer's new method symXModXToKeyQt, to fix numpad keys
dfaure added a comment. I'm completely agree about being careful, as long as it's not the point of not fixing bugs :-) If you can think of more shortcuts that I should check, I'm happy to add them to the unittest and test them with kglobalaccel. REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D6234 To: dfaure, graesslin Cc: #frameworks
D6233: KKeyServer: fix handling of KeypadModifier.
dfaure added inline comments.
INLINE COMMENTS
> graesslin wrote in kkeyserver.cpp:976-980
> This is a special handling from KGlobalAccel. Are you sure it belongs into
> the generic implementation?
I would say yes, because e.g. Ctrl+& (on qwerty) is going to include Shift no
matter which application is calling this method, and it should be removed (just
like Qt does when recording "Ctrl+&" rather than "Ctrl+Shift+&" or
"Ctrl+Shift+7".
But anyway in practice the only other caller of this method is kwin, and it
only cares for [Ctrl|Alt|None]+[Left|Right|Up|Down|Space|Return|Escape], if I
read the code correctly.
See https://lxr.kde.org/ident?_i=xcbKeyPressEventToQt&_remember=1
and AbstractClient::keyPressEvent in kwin.
So, while technically this code could stay in kglobalaccel, it still seems more
correct to me to have it here in the generic "xcb key event to keyQt
conversion" code (and this avoids calling xcb_key_symbols_alloc again in
kglobalaccel).
> graesslin wrote in kkeyserver_x11.h:161
> xkb has the modifier mask defined as uint32_t:
>
> typedef uint32_t xkb_mod_mask_t
strange, I see this:
typedef struct xcb_key_press_event_t {
[...]
uint16_tstate; /**< */
[...]
and `grep modifiers /usr/include/xcb/xproto.h| grep uint` shows consistent use
of uint16_t.
xkb_mod_mask_t seems to come from xcbcommon which is unrelated to the APIs used
here, right?
REPOSITORY
R278 KWindowSystem
REVISION DETAIL
https://phabricator.kde.org/D6233
To: dfaure, graesslin
Cc: graesslin, #frameworks
D6255: KKeySequenceItem: make it possible to record Ctrl+Num+1 as a shortcut.
davidedmundson created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY KQuickControls contains a copy of KSequenceWidget but with the UI handled by a QtQuickControls based QML file. This syncs the relevant part of the changes from https://phabricator.kde.org/D6226 TEST PLAN Ran test to show widget. Could press combos REPOSITORY R296 KDeclarative BRANCH master REVISION DETAIL https://phabricator.kde.org/D6255 AFFECTED FILES src/qmlcontrols/kquickcontrols/private/keysequencehelper.cpp To: davidedmundson Cc: #frameworks
D6233: KKeyServer: fix handling of KeypadModifier.
graesslin added inline comments.
INLINE COMMENTS
> kkeyserver.cpp:976-980
> +if (*keyQt != Qt::Key_Tab) { // KKeySequenceWidget does not map
> shift+tab to backtab
> +static const int FirstLevelShift = 1;
> +keySymX = xcb_key_symbols_get_keysym(symbols, e->detail,
> FirstLevelShift);
> +KKeyServer::symXModXToKeyQt(keySymX, keyModX, keyQt);
> +}
This is a special handling from KGlobalAccel. Are you sure it belongs into the
generic implementation?
> kkeyserver_x11.h:161
> + */
> +KWINDOWSYSTEM_EXPORT bool symXModXToKeyQt(uint32_t keySym, uint16_t modX,
> int *keyQt);
>
xkb has the modifier mask defined as uint32_t:
typedef uint32_t xkb_mod_mask_t
REPOSITORY
R278 KWindowSystem
REVISION DETAIL
https://phabricator.kde.org/D6233
To: dfaure, graesslin
Cc: graesslin, #frameworks
D6234: KGlobalAccel: port to KKeyServer's new method symXModXToKeyQt, to fix numpad keys
graesslin added a comment. > Man it's demotivating to contribute to KDE. Users say all sorts of bad things about KDE, and then future-ex-maintainers reject your contribution. Great. Yes sure, and you can imagine how many angry mails and bug reports I get when things break. I made the experience that touching our X11 implementations always results in regressions. And nobody notices them in the testing period. Unfortunately I can give you a long list of regression we had in Plasma 5.10 only on X11. This makes me being extremely conservative when it comes to changing the X11 code base. The number of developers fully understanding X11 is really low in our community and most of them are working on Wayland now. Nobody is going to detect if we introduce a regression in kglobalaccel prior to the release. And due to the nature of the frameworks distros will ship them and then we as the Plasma team have yet another shit storm because we have a broken global shortcuts handling. So yes I'm extremely careful when it comes to touching this code base. INLINE COMMENTS > dfaure wrote in kglobalaccel_x11.cpp:278-287 > I did not change one inch of that logic, I just moved it to > KKeyServer::xcbKeyPressEventToQt. I'm sorry I didn't notice. I first noticed this review and it has no dependency set. It looked like the code got dropped. REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D6234 To: dfaure, graesslin Cc: #frameworks
