Re: Post-MegaRelease projects

[Vlad Zahorodnii]

On 2/22/24 23:57, Nate Graham wrote:

Hello everyone,

Congrats to the entire KDE community on the impending launch of the 
KDE 6 MegaRelease! I'm so impressed with how folks came together to 
make it amazing. It's a very impressive release and I think people are 
gonna love it.


I've started pondering post-megarelease projects. We've spent so long 
on porting and bugfixing that I think it might be useful to shift 
gears to feature work, and I'd like to brainstorm potential 
large-scale projects and gauge the level of interest in putting 
resources into them soon.


Here are some ideas of mine to get the creative juices started:

* David's input method playground stuff [1] is amazing and needs to be 
developed and productized
* GNOME's Libadwaita app platform has been a runaway success for them; 
evaluate our offerings in comparison and see what we can do better

* Unified theming infrastructure for KDE apps, GTK apps, and Plasma.
** Relatedly: QML/JS in themes is dangerous; move away from it
* Start adding release notes to our apps' AppStream metadata [2]
* Finish up and ship the new Breeze icons
* HIG is outdated and mostly ignored, and needs an overhaul to make it 
useful

* Telemetry system has not proved to be very useful and needs an overhaul
* store.kde.org is full of low-quality or broken content; make a push 
for KDE people to take ownership of content moderation, QA, etc. Also 
any relevant and needed tech improvements

* Our virtual keyboard situation is not great and needs focused work
* KWallet needs an overhaul
* Have KWin (optionally) remember window positions on Wayland
* Build a "System misconfiguration detection hub" app [3]

Feel free to discuss, and propose your own!

Nate



[1] 
https://blog.davidedmundson.co.uk/blog/new-ideas-using-wayland-input-methods/

[2] https://github.com/ximion/appstream/issues/354
[3] https://invent.kde.org/plasma/plasma-workspace/-/issues/64


Speaking for kwin, there are a few things that I would like us to 
complete or spend more time on


- Fractional scaling
- Output layers
- Scene: port as much as possible stuff to the Item. It would be nice to 
add support for animating Items in order to start phasing out 
AnimationEffect. Also, as we discussed in #kwin, it's worth looking into 
splitting item and render node trees

- Input: input grabs & redirection of input through the scene
- Split kwin_wayland and kwin_x11 + further design cleanups and code 
shuffling

- Explicit sync
- Triple buffering
- Proper output mirror and tiled output support
- Try enabling color management by default

The list is quite ambitious for 6.1 as it requires a lot of work.

Re: Splitting KGlobalAccel interface and runtime

[Vlad Zahorodnii]

On 2/13/23 22:05, Nicolas Fella wrote:

Hi,

the kglobalaccel framework currently contains two pieces: kglobalacceld,
the runtime component that manages global shortcuts, and an
application-side library to interact with it.

The two communicate with each other via DBus. On X11 there is a
standalone kglobalacceld5 process providing the interface, on Wayland
the runtime is linked into KWin and thus the kwin_wayland process
provides the interface.

The current architecture has a number of downsides:

- Any call to the KGlobalAccel library may DBus-activate the
kglobalacceld5 process, which may be undesired on Desktop other than
Plasma since it competes with their native shortcut system. We tried
fixing that by making such calls no-op on !Plasma, but that broke things
for people that did want it to run, for example people using KWin with
LXQt, because KWin relies on KGlobalAccel for shortcuts

- We want to keep the dependencies of the interface library minimal,
which is inconvenient for the development of the runtime part. For
example we really want to use KIO::ApplicationLauncherJob in the
runtime, but currently can't, because that would introduce a dependency
cycle in Frameworks (KIO depends on KXmlGui, which depends on
KGlobalAccel, which would depend on KIO)

- Coinstallability of KF5 and KF6. Conceptually there can only be one
kglobalacceld. If both KF5 KGlobalAccel and KF6 KGlobalAccel install a
kglobalacceld this is going to be problematic. This also means that a
KF6-based kglobalacceld must work with a KF5 interface library

- Other shortcut daemon implementations. Since somewhat recently there
is an XDG Portal for global shortcuts. Platforms like Windows and macOS
also have ways for applications to register global shortcuts. While we
are currently not using any of these it's very well possible that we
would eventually want to use the KGlobalAccel interface library to
interact with those. Having the kglobalacceld runtime in the same
frameworks therefore doesn't feel right.

To address these issues I suggest we split out the runtime part of
kglobalaccel into its own project, under the Plasma release group,
because that's primarily where it's used/supported. The interface
library would remain in frameworks. We have a similar situation with
activities, where the manager (kactivitymanagerd) is in Plasma and the
interface is in Frameworks. When doing this we'd also change the way how
kglobalacceld is started away from DBus activation towards explicitly
starting it as part of the Plasma startup. This avoids accidentally
launching it when it shouldn't be but still allows to explicitly start
outside of Plasma if really wanted. It would also allow for greater
flexibility in the development of the runtime, especially around
dependency constraints.

It wouldn't automatically solve the coinstallability problem of KF5 and
KF6, because a kglobalacceld provided by KF5-KGlobalAccel would still
conflict with a Plasma-provided kglobalacceld, but it's at least
conceptually less messy since it's clear that the Plasma-provided one
would be the preferred one to use.

Thoughts about this?


+1

There's one caveat though: given that the library and the runtime parts 
will have different release schedules, we will have to be careful about 
protocol changes. Perhaps we could borrow a thing or two from activities?


Regards,
Vlad

Re: "Gardening" old bugreports

[Vlad Zahorodnii]

On 1/19/23 02:35, Nicolas Fella wrote:> Properly cleaning up old 
bugreports is important. However, it requires

some level of care and expertise to judge whether a bugreport is still
useful. Judging by the volume of bugreports that is pinged with the same
copy message this care is not  applied here.


For projects like kwin it's unrealistic to go through all bug reports. 
Just for the record, there are currently almost 1.3k open bug reports 
filed for kwin.


Going through old bug reports is not fun! After triaging about 20 bug 
reports, I already feel exhausted. It's a time and energy consuming 
process that kills future motivation to do bug triaging again. Now scale 
that to 1000 bug reports!


I have no doubt that there are old bug reports that are actually still 
valid and useful. But, from my personal experience, the ratio of such 
good bug reports to "noise" is small.


> Improperly applied such mass changes can do more harm than good. We may
> close bugreports that are actually still useful just because nobody
> replied on then in a relatively short timeframe.

IMHO it makes sense to poke old bug reports (say 1 year old or so)

My two cents,
Vlad

Re: KWayland respin request

[Vlad Zahorodnii]

On 9/29/22 00:22, David Faure wrote:

Hi Vlad,

I just read your email now. Better email me or release-team@ with such
requests, for faster reaction times. In any case, 5.98.0 was released on Sep
12, so your mail came after the release, so it's not like a "respin" of 5.98.0
was possible.

A 5.98.1 release of kwayland would be possible though. But I'm not sure it's
worth it at this point, 5.99.0 will be tagged in 3 days and released in 10
days.


Yeah, I think it's not as important as it used to be because 5.99.0 will 
be released soon.


Regards,
Vlad

KWayland respin request

[Vlad Zahorodnii]

Hello,

Is there any chance of a kwayland respin with 
https://invent.kde.org/frameworks/kwayland/-/commit/d02188ad1f615adcf842f4c9806ba9e62ccb 
included. It should fix misplaced plasma panel popups in Plasma Wayland 
session.


Regards,
Vlad

Re: kdesrc -- stuck building kwayland

[Vlad Zahorodnii]

Update wayland package. You need at least 1.20. Seems like we need to 
update CMakeLists.txt in kwayland


On 9/12/22 16:49, Nicola Mingotti wrote:

yes, sure, I attach it here

Nicola



On Mon, Sep 12, 2022 at 3:28 PM Vlad Zahorodnii <mailto:vlad.zahorod...@kde.org>> wrote:


On 9/12/22 16:07, Nicola Mingotti wrote:
 > Hi !
 >
 > I am very new to KDE development. I was doing some experiments with
 > *kdesrc*. I successfully built and modified dolphin, adding
features i
 > wanted, of which i am extremely enthusiastic!
 >
 > I am using 10h/day Debian Stable since many years and on several
 > severs, it would beneficial to me to be able to keep working with
this
 > OS and not move to a rolling-release OS. So, kdesrc for me it is
great
 > to try new KDE software.
 >
 > Unfortunately it is a few days i am not able to compile
 > anything else since *kwayland* blocks all other elements.
 >
 > Trying to:
 > $> kdesrc-build kwayland --refresh-build
 > it fails, giving me back:
 > -
 > Building kwaylandfrom frameworks(3/3)
 >     Fetching remote changes to kwayland
 >     Merging kwaylandchanges from branch master
 >     Source update complete for kwayland: no files affected
 >   Rebuilding because the option refresh-build was set
 >     Preparing build system for kwayland.
 >     Removing files in build directory for kwayland
 >     Old build system cleaned, starting new build system.
 >     Running cmaketargeting Unix Makefiles...
 >     Compiling... failed(after 38 seconds)
 >     Note: --- 27--- compile warnings
 >
 > kwayland didn't build, stopping here.
 >
 > <<<  PACKAGES FAILED TO BUILD  >>>
 > kwayland- file:///home/p/kde/src/log/2022-09-12-03/kwayland/build.log
 >
 > Important notification for kwayland:
 > kwaylandhas failed to build 8times.
 >     You can check https://build.kde.org/search/?q=kwayland
<https://build.kde.org/search/?q=kwayland>
 > <https://build.kde.org/search/?q=kwayland
<https://build.kde.org/search/?q=kwayland>> to see if this is expected.
 > ---
 >
 > . looking into the "build.log" file:
 > 
 > ...
 >
 > [ 46%] Building C object

src/server/CMakeFiles/KF5WaylandServer.dir/wayland-linux-dmabuf-unstable-v1-protocol.c.o
 > [ 46%] Building C object

src/server/CMakeFiles/KF5WaylandServer.dir/wayland-tablet-unstable-v2-protocol.c.o
 > [ 46%] Building CXX object

src/server/CMakeFiles/KF5WaylandServer.dir/qwayland-server-tablet-unstable-v2.cpp.o
 > [ 47%] Linking CXX shared library ../../bin/libKF5WaylandServer.so
 > [ 47%] Built target KF5WaylandServer
 > gmake: *** [Makefile:160: all] Error 2
 >
 > ---
 >
 > . if i follow the link:
 > https://build.kde.org/search/?q=kwayland
<https://build.kde.org/search/?q=kwayland>
 > <https://build.kde.org/search/?q=kwayland
<https://build.kde.org/search/?q=kwayland>>
 > i get a Grafana page asking me a password.
 >
 > . I don't know how to find the file "Makefile" of which
 > "gmake" is talking about. And "error 2" gives me zero hints :/
 >
 > . I recently run
 > $> sudo apt-get update
 > $> sudo apt upgrade
 > => no difference on compiling kwayland, same error output
 >
 > I am stuck, any ideas ?
 >
 > bye
 > Nicola

Can you post the full build.log file?

Regards,
Vlad

Re: kdesrc -- stuck building kwayland

[Vlad Zahorodnii]

On 9/12/22 16:07, Nicola Mingotti wrote:

Hi !

I am very new to KDE development. I was doing some experiments with 
*kdesrc*. I successfully built and modified dolphin, adding features i 
wanted, of which i am extremely enthusiastic!


I am using 10h/day Debian Stable since many years and on several
severs, it would beneficial to me to be able to keep working with this 
OS and not move to a rolling-release OS. So, kdesrc for me it is great 
to try new KDE software.


Unfortunately it is a few days i am not able to compile
anything else since *kwayland* blocks all other elements.

Trying to:
$> kdesrc-build kwayland --refresh-build
it fails, giving me back:
-
Building kwaylandfrom frameworks(3/3)
    Fetching remote changes to kwayland
    Merging kwaylandchanges from branch master
    Source update complete for kwayland: no files affected
  Rebuilding because the option refresh-build was set
    Preparing build system for kwayland.
    Removing files in build directory for kwayland
    Old build system cleaned, starting new build system.
    Running cmaketargeting Unix Makefiles...
    Compiling... failed(after 38 seconds)
    Note: --- 27--- compile warnings

kwayland didn't build, stopping here.

<<<  PACKAGES FAILED TO BUILD  >>>
kwayland- file:///home/p/kde/src/log/2022-09-12-03/kwayland/build.log

Important notification for kwayland:
kwaylandhas failed to build 8times.
    You can check https://build.kde.org/search/?q=kwayland 
 to see if this is expected.

---

. looking into the "build.log" file:

...

[ 46%] Building C object 
src/server/CMakeFiles/KF5WaylandServer.dir/wayland-linux-dmabuf-unstable-v1-protocol.c.o
[ 46%] Building C object 
src/server/CMakeFiles/KF5WaylandServer.dir/wayland-tablet-unstable-v2-protocol.c.o
[ 46%] Building CXX object 
src/server/CMakeFiles/KF5WaylandServer.dir/qwayland-server-tablet-unstable-v2.cpp.o
[ 47%] Linking CXX shared library ../../bin/libKF5WaylandServer.so
[ 47%] Built target KF5WaylandServer
gmake: *** [Makefile:160: all] Error 2

---

. if i follow the link:
https://build.kde.org/search/?q=kwayland 


i get a Grafana page asking me a password.

. I don't know how to find the file "Makefile" of which
"gmake" is talking about. And "error 2" gives me zero hints :/

. I recently run
$> sudo apt-get update
$> sudo apt upgrade
=> no difference on compiling kwayland, same error output

I am stuck, any ideas ?

bye
Nicola


Can you post the full build.log file?

Regards,
Vlad

D24629: RFC: Introduce KClockSkewNotifier class

[Vlad Zahorodnii]

zzag abandoned this revision.

REPOSITORY
  R244 KCoreAddons

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

To: zzag, #frameworks
Cc: apol, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ahmadsamir, ngraham, bruns, vkrause

Re: Plasma on Wayland VirtualBox guest screen resize

[Vlad Zahorodnii]

On a bit related note, this mailing list is mostly about core stuff, 
e.g. announcing moving projects to kdereview, etc. We can continue this 
discussion in the kwin mailing list k...@kde.org, you can also reach out 
kwin devs in #kde-kwin on Libera.Chat or matrix.


Cheers,
Vlad

Re: Plasma on Wayland VirtualBox guest screen resize

[Vlad Zahorodnii]

On 2/1/22 22:58, Vadim Galitsin wrote:

Hi guys,

I am currently looking to the issue with VirtualBox guest screen 
resizing when guest is running Plasma on Wayland (using VMSVGA graphics 
configuration).


For my experiments, I use Kubuntu 22.04 guest with Plasma 5.23. Guest 
Additions are from our latest development build 
(https://www.virtualbox.org/download/testcase/VBoxGuestAdditions_6.1.97-149665.iso).


In general, screen resizing does not work. If I, for example, drag 
right-bottom corner of the guest window, I can see that guest screen 
up-scales, but does not increase in size. So, it looks like a zoom-in 
effect when image size stays the same.


For resizing on a guest side, we are using tool called VBoxDRMClient 
(https://www.virtualbox.org/browser/vbox/trunk/src/VBox/Additions/x11/VBoxClient/display-drm.cpp). 
This tool receives resize hints from host and injects them into vmwgfx 
driver using custom ioctl DRM_IOCTL_VMW_UPDATE_LAYOUT. This approach 
works for X11 Desktop Environments as well as for Gnome3 on Wayland. 
However, not for Plasma case.


What do you think if we could take a look to the issue from both sides? 
If there is something in your mind what VBoxDRMClient is potentially 
missing to have better integration with KDE/Plasma, we could discuss that.


Best regards,
Vadim


Hi,

There was some work in 5.23 to improve VM integration, but it looks like 
there are still some issues.. As long as kwin_wayland receives a change 
udev event after the vm window has been resized, kwin should re-scan drm 
connectors, reload modes, and resize the workspace to match the new 
output layout.


Relevant code is here 
https://invent.kde.org/plasma/kwin/-/blob/749cf798ce465634b616682e7c072f0a43a22906/src/backends/drm/drm_backend.cpp#L254. 
It's worth checking if kwin receives the relevant udev event.


Cheers,
Vlad

Re: Marge bot integration

[Vlad Zahorodnii]

On 1/24/22 12:14, Rohan Garg wrote:

Mesa currently uses marge-bot extensively to merge contributions from
developers. It definitely simplified a lot of the contribution
workflow for developers.


Yeah, I discovered marge-bot thanks to Mesa. :-)

Cheers,
Vlad

Re: Marge bot integration

[Vlad Zahorodnii]

On 1/24/22 13:02, Ben Cooksley wrote:

This is very similar to the “Merge Train” feature in GitLab
Enterprise, right?


Correct, although it has more functionality in some areas from my 
understanding of it.


Yeah, after quickly reading gitlab docs, it looks exactly what we look for.

Regards,
Vlad

Marge bot integration

[Vlad Zahorodnii]

Hi,

In kwin, we experience merge congestion issues where pending merge is 
aborted because another merge request has been merged in meanwhile, so 
you need to go back to the MR, rebase it and schedule a merge again. 
This is exhausting and frustrating developer experience.


Fortunately, this kind of issues can be solved by using 
https://github.com/smarkets/marge-bot. With marge bot, when all review 
comments have been addressed, you need to assign the MR to the marge-bot 
user. After that, it will try to merge the patch and if needed rebase 
and retry again. marge-bot is used by projects such as Mesa, pulse 
audio, various GNOME projects, etc.


I think that marge-bot would make merging patches in projects with high 
traffic such as plasma a lot easier. Thoughts?


Regards,
Vlad

Re: KDE CI: Plasma » plasma-framework » kf5-qt5 FreeBSDQt5.15 - Build # 644 - Still Failing!

[Vlad Zahorodnii]

On 10/18/21 14:25, Aleix Pol wrote:
Does anyone know why we are having this problem all of a sudden? I 
cannot reproduce on my system.

Plasma Workspace is failing the same:
https://build.kde.org/job/Plasma/job/plasma-workspace/job/kf5-qt5%20SUSEQt5.15/1800/ 



It might be that plasma-framework is built with older kcoreaddons.

> 
/usr/home/jenkins/install-prefix/include/KF5/KCoreAddons/kpluginmetadata.h:443:54:

> note: passing argument to parameter 'defaultValue' here
> [2021-10-18T11:17:43.890Z] QString value(const QString , const
> QString  = QString()) const;

It appears like the compiler chooses wrong overload.

Re: Gitlab CI - Inbound

[Vlad Zahorodnii]

On 9/7/21 1:21 PM, Ben Cooksley wrote:
On Tue, Sep 7, 2021 at 10:14 PM Vlad Zahorodnii <mailto:vlad.zahorod...@kde.org>> wrote:


On 9/7/21 12:22 PM, Ben Cooksley wrote:
 > On Tue, Sep 7, 2021 at 8:48 PM Vlad Zahorodnii
mailto:vlad.zahorod...@kde.org>
 > <mailto:vlad.zahorod...@kde.org
<mailto:vlad.zahorod...@kde.org>>> wrote:
 >
 >     On 9/5/21 3:18 PM, David Faure wrote:
 >      > On dimanche 5 septembre 2021 12:26:50 CEST Ben Cooksley wrote:
 >      >> On Sun, Sep 5, 2021 at 10:22 PM David Faure
mailto:fa...@kde.org>
 >     <mailto:fa...@kde.org <mailto:fa...@kde.org>>> wrote:
 >      >>> For frameworks, I think we should be able to write a
one-time
 >     script that
 >      >>> generates .kde-ci.yml files using the dependencies listed in
 >     kde-build-
 >      >>> metadata (and the platforms listed in metainfo.yaml) ?
 >      >>
 >      >> Yes, that should work nicely (although that information
now lives in
 >      >> sysadmin/repo-metadata, dependencies folder)
 >      >
 >      > All done for Frameworks.
 >      >
 >      > The script that collects platforms from metainfo.yaml won't be
 >     useful for
 >      > other KDE modules, but the script that collects deps from
 >     dependency-data-kf5-
 >      > qt5 is attached, in case it's useful to anyone else.
 >
 >     Is there a file that maps legacy project paths to gitlab
project paths?
 >     dependency-data-kf5-qt5 lists projects with their legacy paths.
 >
 >
 > The individual project YAML files in sysadmin/repo-metadata
contain this
 > information.
 > The legacy project path can be found under 'projectpath' while the
 > Gitlab paths are under 'repopath'

Thanks! I've attached a quick and dirty python script that parses
project dependencies from repo-metadata and prints corresponding gitlab
project paths.

Example usage

    python project-deps.py --repo-metadata
/data/projects/src/repo-metadata/ kde/workspace/kwin

Output

Dependencies:
    'requires':
      'frameworks/extra-cmake-modules': '@stable'
      'plasma/kdecoration': '@stable'
      'plasma/kscreenlocker': '@stable'
      'plasma/kwayland-integration': '@stable'
      'plasma/breeze': '@stable'
      'plasma/kwayland-server': '@stable'


Please note that the above is missing a 'on' section as required for 
each Dependencies section in the .kde-ci.yml file.
Yeah... Is there a database or something that can be queried to fill in 
the "on" section?

Re: Gitlab CI - Inbound

[Vlad Zahorodnii]

On 9/7/21 12:22 PM, Ben Cooksley wrote:
On Tue, Sep 7, 2021 at 8:48 PM Vlad Zahorodnii <mailto:vlad.zahorod...@kde.org>> wrote:


On 9/5/21 3:18 PM, David Faure wrote:
 > On dimanche 5 septembre 2021 12:26:50 CEST Ben Cooksley wrote:
 >> On Sun, Sep 5, 2021 at 10:22 PM David Faure mailto:fa...@kde.org>> wrote:
 >>> For frameworks, I think we should be able to write a one-time
script that
 >>> generates .kde-ci.yml files using the dependencies listed in
kde-build-
 >>> metadata (and the platforms listed in metainfo.yaml) ?
 >>
 >> Yes, that should work nicely (although that information now lives in
 >> sysadmin/repo-metadata, dependencies folder)
 >
 > All done for Frameworks.
 >
 > The script that collects platforms from metainfo.yaml won't be
useful for
 > other KDE modules, but the script that collects deps from
dependency-data-kf5-
 > qt5 is attached, in case it's useful to anyone else.

Is there a file that maps legacy project paths to gitlab project paths?
dependency-data-kf5-qt5 lists projects with their legacy paths.


The individual project YAML files in sysadmin/repo-metadata contain this 
information.
The legacy project path can be found under 'projectpath' while the 
Gitlab paths are under 'repopath'


Thanks! I've attached a quick and dirty python script that parses 
project dependencies from repo-metadata and prints corresponding gitlab 
project paths.


Example usage

  python project-deps.py --repo-metadata 
/data/projects/src/repo-metadata/ kde/workspace/kwin


Output

Dependencies:
  'requires':
'frameworks/extra-cmake-modules': '@stable'
'plasma/kdecoration': '@stable'
'plasma/kscreenlocker': '@stable'
'plasma/kwayland-integration': '@stable'
'plasma/breeze': '@stable'
'plasma/kwayland-server': '@stable'

Cheers,
Vlad




Cheers,
Ben


 >> Once we've transitioned across to the .kde-ci.yml files the existing
 >> dependency metadata and branch group files will no longer be
required.
 >> (We'll need to work out a way of exporting this information for easy
 >> consumption by kdesrc-build and other clients before it can be
removed
 >> though)
 >
 > OK. Duplication is bad so I agree :)
 >



#!/usr/bin/env python3

import argparse
import glob
import os
import yaml


def build_legacy_to_gitlab_table(repo_metadata):
table = dict()
repo_root = os.path.join(repo_metadata, "projects-invent/**/**/metadata.yaml")
for filename in glob.glob(repo_root):
with open(filename, "r") as f:
document = yaml.safe_load(f)
legacy_path = document["projectpath"]
gitlab_path = document["repopath"]
if not legacy_path:
print(filename + " is invalid")
elif not gitlab_path:
print(filename + " is invalid")
else:
table[legacy_path] = gitlab_path
return table


def parse_dependencies(repo_metadata, legacy_project_path):
deps_file = os.path.join(repo_metadata, "dependencies", "dependency-data-kf5-qt5")
dependencies = list()
needle = legacy_project_path + ":"
with open(deps_file, "r") as f:
for line in f:
if line.startswith("*:"):
dep = line[len("*:"):].strip()
elif line.startswith(needle):
dep = line[len(needle):].strip()
else:
continue
if dep.startswith("-"):
dep = dep[1:]
dependencies.remove(dep)
else:
dependencies.append(dep)
return dependencies


def print_dependencies(mapping_table, dependencies):
gitlab_deps = list()
for dependency in dependencies:
if dependency in mapping_table:
gitlab_deps.append(mapping_table[dependency])
elif dependency != "third-party/Qt5":
print(dependency + " has no matching gitlab repo")
if gitlab_deps:
print("Dependencies:")
print("  'requires':")
for dep in gitlab_deps:
print(f"'{dep}': '@stable'")


parser = argparse.ArgumentParser(description='kde ci dependency builder')
parser.add_argument('--repo-metadata',
help='the file path to sysadmin/repo-metadata repo')
parser.add_argument('projectpath', help='projectpath')
args = parser.parse_args()

repo_metadata = args.repo_metadata
projectpath = args.projectpath

mapping_table = build_legacy_to_gitlab_table(repo_metadata)
legacy_dependencies = parse_dependencies(repo_metadata, projectpath)
print_dependencies(mapping_table, legacy_dependencies)

Re: Gitlab CI - Inbound

[Vlad Zahorodnii]

On 9/5/21 3:18 PM, David Faure wrote:

On dimanche 5 septembre 2021 12:26:50 CEST Ben Cooksley wrote:

On Sun, Sep 5, 2021 at 10:22 PM David Faure  wrote:

For frameworks, I think we should be able to write a one-time script that
generates .kde-ci.yml files using the dependencies listed in kde-build-
metadata (and the platforms listed in metainfo.yaml) ?


Yes, that should work nicely (although that information now lives in
sysadmin/repo-metadata, dependencies folder)


All done for Frameworks.

The script that collects platforms from metainfo.yaml won't be useful for
other KDE modules, but the script that collects deps from dependency-data-kf5-
qt5 is attached, in case it's useful to anyone else.


Is there a file that maps legacy project paths to gitlab project paths? 
dependency-data-kf5-qt5 lists projects with their legacy paths.


vlad


Once we've transitioned across to the .kde-ci.yml files the existing
dependency metadata and branch group files will no longer be required.
(We'll need to work out a way of exporting this information for easy
consumption by kdesrc-build and other clients before it can be removed
though)


OK. Duplication is bad so I agree :)

Re: Including LayerShellQt in Plasma in time for 5.22

[Vlad Zahorodnii]

Hi,

On 4/3/21 1:26 PM, Albert Astals Cid wrote:

clang complains that

/home/tsdgeos/devel/kde/layer-shell-qt/src/qwaylandlayershell_p.h:23:24: note: 
did you mean class here?
 QWaylandLayerShell(struct QtWayland::zwlr_layer_shell_v1 *shell);
^~
class


https://invent.kde.org/plasma/layer-shell-qt/-/merge_requests/2 should 
fix it.



clazy complains that
/home/tsdgeos/devel/kde/layer-shell-qt/src/qwaylandlayersurface.cpp:72:39: error: 
Pass small and trivially-copyable type by value (const class QMargins &) 
[-Wclazy-function-args-by-value]
void QWaylandLayerSurface::setMargins(const QMargins )


In fairness, a lot of plasma projects pass QMargins objects via const 
ref and it's hard to tell what Qt types are trivially copyable.



There are two TODO in the code, how important they are, should they be done 
before release?


Some of those TODO comments require changes in QtWayland, which arguably 
can be done only in the Qt 6 timeframe.


Cheers,
Vlad

Re: adding support for KGlobalAccel in Clementine?

[Vlad Zahorodnii]

Howdy,

On 8/1/20 12:32 PM, Alan Ezust wrote:
> But I am not sure exactly what to do to the cmakelists.txt of Clementine
> in order to "bring in" this framework into Clementine.
> 
> I know I need to put a find_package(KF5GlobalAccel) somewhere in it, but
> what else do I need to do to start using this library ?

That's right, you need to find KF5GlobalAccel first

find_package(KF5GlobalAccel CONFIG)

and then link your program against KF5::GlobalAccel

target_link_libraries(target_name KF5::GlobalAccel)

Cheers,
Vlad

Policy on forward declarations for things from external libraries

[Vlad Zahorodnii]

Howdy,

From time to time, I find myself in a situation where a code reviewer 
suggests to replace #include  with the corresponding class 
forward declaration. Such discussions usually get us nowhere because 
neither the code reviewer nor I mind to seek for a compromise. A policy 
would prevent wasting time on such discussions.


Note that I used Qt/QFoobar only as an example. Similar logic can be 
applied to other libraries as well. For example, libxcb or libexif, etc.


The good thing about forward declarations is that they can reduce the 
compilation time quite significantly.


The bad thing about it is that we must guarantee that the definition of 
QFoobar in the external library won't change. For example, QFoobar won't 
move into an inline namespace all of sudden, etc.


I propose to add a paragraph in [1] saying whether it is okay to add 
forward declarations for things from external libraries.


Any thoughts or suggestions?

Cheers,
Vlad

https://community.kde.org/Policies/Library_Code_Policy#Avoid_including_other_headers_in_headers

D26503: [Dialog Shadows] Port to KWindowSystem shadows API

[Vlad Zahorodnii]

zzag closed this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: zzag, #plasma, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D26503: [Dialog Shadows] Port to KWindowSystem shadows API

[Vlad Zahorodnii]

zzag edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  port-to-shadows-api

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

To: zzag, #plasma, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D26503: [Dialog Shadows] Port to KWindowSystem shadows API

[Vlad Zahorodnii]

zzag updated this revision to Diff 83261.
zzag added a comment.


  Fix merge conflict.

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26503?vs=74590=83261

BRANCH
  port-to-shadows-api

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

AFFECTED FILES
  src/plasmaquick/dialogshadows.cpp
  src/plasmaquick/dialogshadows_p.h
  src/plasmaquick/waylandintegration.cpp
  src/plasmaquick/waylandintegration_p.h

To: zzag, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29774: Add X-KDE-DBUS-Restricted-Interfaces to Application desktop entry fields

[Vlad Zahorodnii]

zzag added a comment.


  > This added field to Application desktop entries allows to declare an access 
request to a DBUS interface.
  
  Just to be sure. Does X-KDE-DBUS-Restricted-Interfaces indicate a D-Bus 
interface or a D-Bus service?

REPOSITORY
  R309 KService

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

To: meven, davidedmundson, zzag, #frameworks, #kwin, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29815: Fix blurry icons in titlebar appmenu by adding UseHighDpiPixmaps flag

[Vlad Zahorodnii]

zzag added a comment.


  In D29815#672682 , @anthonyfieroni 
wrote:
  
  > Is that KWin that set titlebar menus?
  
  
  from `#plasma` irc
  
  5:02:41 PM  kded does not load kwin
  5:03:15 PM  I believe the appmenu version where it's shown in the title 
bar is run by kded
  5:03:28 PM  kwin just triggers a qmenu shown at an explicit x,y
  5:04:42 PM  motivation at the time was to keep kwin from having too 
much random code in it... a design philosophy that hasn't aged well!

REPOSITORY
  R297 KDED

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

To: mthw, #frameworks, zzag
Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D29815: Fix blurry icons in titlebar appmenu by adding UseHighDpiPixmaps flag

[Vlad Zahorodnii]

zzag removed a reviewer: zzag.

REPOSITORY
  R297 KDED

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

To: mthw, #frameworks
Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D29569: Fix computing display geometry on multi-monitor HiDPI setups on X11

[Vlad Zahorodnii]

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


  In general, it's better to use QScreen::availableGeometry().

REPOSITORY
  R278 KWindowSystem

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

To: printesoi, davidedmundson, #kwin, zzag
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29256: [server] Introduce mapped() signal

[Vlad Zahorodnii]

zzag closed this revision.

REPOSITORY
  R127 KWayland

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

To: zzag, #kwin, davidedmundson, apol
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29278: Port KWin to KWaylandServer

[Vlad Zahorodnii]

zzag accepted this revision.

REPOSITORY
  R108 KWin

BRANCH
  master

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

To: apol, #kwin, #plasma, #frameworks, davidedmundson, zzag
Cc: zzag, kwin, Orage, cacarry, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, mkulinski, ragreen, jackyalcine, iodelay, crozbo, bwowk, 
ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, hardening, 
romangg, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29278: Port KWin to KWaylandServer

[Vlad Zahorodnii]

zzag added a comment.


  Hmm, I can't build kwin
  

/home/vlad/Workspace/KDE/src/kde/workspace/kwin/libkwineffects/kwineffects.cpp:44:10:
 fatal error: KWaylandServer/surface_interface.h: No such file or directory
   44 | #include 
  |  ^~~~
compilation terminated.

INLINE COMMENTS

> CMakeLists.txt:131
> +TYPE REQUIRED
> +PURPOSE "For screenlocker integration in kwin_wayland"
> +)

Well, we need it to make the wayland session work.

> CMakeLists.txt:644
>  KF5::WaylandClient
> -KF5::WaylandServer
> +Plasma::KWaylandServer
>  Wayland::Cursor

I feel like it should be KWaylandServer::KWaylandServer but we can revisit it.

> kwineffects.h:68
>  
> -namespace KWayland {
> -namespace Server {
> -class SurfaceInterface;
> -class Display;
> -}
> +namespace KWaylandServer {
> +class SurfaceInterface;

This breaks source compatibility, but I guess we don't have a choice.

REPOSITORY
  R108 KWin

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

To: apol, #kwin, #plasma, #frameworks
Cc: zzag, kwin, Orage, cacarry, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, mkulinski, ragreen, jackyalcine, iodelay, crozbo, bwowk, 
ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, hardening, 
romangg, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29231: Add keyboard_shortcuts_inhibit protocol

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> davidedmundson wrote in keyboard_shortcuts_inhibit_interface.cpp:21
> Q_DECL_PRIVATE

You've probably meant Q_DECL_HIDDEN, right?

On an unrelated note: there are valid arguments against nested private classes 
so it would be really nice if we revisit this topic in the new repo.

> keyboard_shortcuts_inhibit_interface.h:61
> + */
> +KeyboardShortcutsInhibitorInterface 
> *getShortcutsInhibitor(SurfaceInterface *surface, SeatInterface *seat) const;
> +

We usually don't prefix getters with "get". What about shortcutsInhibitor() or 
findInhibitior()?

REPOSITORY
  R127 KWayland

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

To: bport, zzag, davidedmundson, apol
Cc: romangg, crossi, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D29256: [server] Introduce mapped() signal

[Vlad Zahorodnii]

zzag marked an inline comment as done.

REPOSITORY
  R127 KWayland

BRANCH
  introduce-mapped-signal

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

To: zzag, #kwin, davidedmundson
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29256: [server] Introduce mapped() signal

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> apol wrote in surface_interface.cpp:333
> I don't understand, ^ and != are logically equivalent, ^ is the bitwise 
> counterpart.
> 
> Am I missing something?

Oh, I thought you suggested to do `source->buffer != target->buffer`.

REPOSITORY
  R127 KWayland

BRANCH
  introduce-mapped-signal

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

To: zzag, #kwin, davidedmundson
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29256: [server] Introduce mapped() signal

[Vlad Zahorodnii]

zzag updated this revision to Diff 81456.
zzag marked 2 inline comments as done.
zzag added a comment.


  Use !=

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29256?vs=81453=81456

BRANCH
  introduce-mapped-signal

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

AFFECTED FILES
  autotests/client/test_wayland_surface.cpp
  src/server/surface_interface.cpp
  src/server/surface_interface.h

To: zzag, #kwin, davidedmundson
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29256: [server] Introduce mapped() signal

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> apol wrote in surface_interface.cpp:333
> Using != would probably be more readable and accurate (we're don't need it to 
> be bitwise, we're assuming bool changes it to 1 or 0).

We can't use != because mapped() will be emitted each time a new buffer is 
attached to the surface.

REPOSITORY
  R127 KWayland

BRANCH
  introduce-mapped-signal

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

To: zzag, #kwin, davidedmundson
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29256: [server] Introduce mapped() signal

[Vlad Zahorodnii]

zzag updated this revision to Diff 81453.
zzag added a comment.


  Check whether the attached buffer flip-flopped between non-null and null only 
when bufferChanged is true.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29256?vs=81432=81453

BRANCH
  introduce-mapped-signal

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

AFFECTED FILES
  autotests/client/test_wayland_surface.cpp
  src/server/surface_interface.cpp
  src/server/surface_interface.h

To: zzag, #kwin, davidedmundson
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29256: [server] Introduce mapped() signal

[Vlad Zahorodnii]

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

REVISION SUMMARY
  In KWin, we need to know when a sub-surface becomes mapped or unmapped
  so we can generate or filter out window quads for the sub-surface.

REPOSITORY
  R127 KWayland

BRANCH
  introduce-mapped-signal

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

AFFECTED FILES
  autotests/client/test_wayland_surface.cpp
  src/server/surface_interface.cpp
  src/server/surface_interface.h

To: zzag, #kwin
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

Re: Splitting KWayland

[Vlad Zahorodnii]

On 4/27/20 4:12 PM, David Edmundson wrote:
I don't think we want to remove client or server tests on this one. As 
the client tests covered the server side too


Hmm, does this mean we are going to keep both the client and the server 
side in KWaylandServer?


Cheers,
Vlad

D29231: [WIP] Add keyboard_shortcuts_inhibit protocol

[Vlad Zahorodnii]

zzag added a comment.


  Thanks, this patch looks good to me. Although it would be nice to see the 
kwin side before leaving a +1. :)

REPOSITORY
  R127 KWayland

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

To: bport, zzag, davidedmundson, apol
Cc: romangg, crossi, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D28882: Create protocol to manage video feeds

[Vlad Zahorodnii]

zzag added a comment.


  In D28882#658780 , @apol wrote:
  
  > > In future, it might be faster to put up just the interface xml for review 
first.
  >
  > @davidedmundson  @zzag I don't really see how it would have made a 
difference, you only decided to review it 7 to 10 days after I first submitted 
it. 路
  >
  > > What about using existing wl_output objects? The add_source event can be 
simplified quite a lot (even maybe dropped). For windows, we could use string 
handles.
  >
  > So your proposal would be to pass a random resource id or string and see 
what the compositor spits back?
  
  
  Well, it's not that different from passsing uints like this patch proposes. 
In either case we have to have some protection against garbage input values and 
possibly inert resources. wl_outputs are not some random objects and 
practically every GUI application keeps track of them.
  
  > My intention was to keep the protocol auto-contained to some extent so 
every client doesn't need to track windows and screens to be able to request a 
stream.

REPOSITORY
  R127 KWayland

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

To: apol, #kwin, jgrulich, davidedmundson, zzag
Cc: meven, davidedmundson, romangg, zzag, kde-frameworks-devel, LeGast00n, 
cblack, michaelh, ngraham, bruns

D28882: Create protocol to manage video feeds

[Vlad Zahorodnii]

zzag requested changes to this revision.
zzag added a comment.
This revision now requires changes to proceed.


  > In future, it might be faster to put up just the interface xml for review 
first.
  
  ++
  
  ---
  
  What about using existing `wl_output` objects? The `add_source` event can be 
simplified quite a lot (even maybe dropped). For windows, we could use string 
handles.
  

  
  
  



  
  
  

  
  Please notice that the manager object also needs to advertise supported 
cursor modes
  

  
  
  



  
  


INLINE COMMENTS

> screencast.xml:8
> +  ]]>
> +
> +

Interfaces usually come without `unstable` in the name.

s/zkde_screencast_unstable_v1/zkde_screencast_v1/

> screencast.xml:52-54
> +
> +
> +

zkde_screencast_stream_unstable_v1 must have a destructor request.

REPOSITORY
  R127 KWayland

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

To: apol, #kwin, jgrulich, davidedmundson, zzag
Cc: meven, davidedmundson, romangg, zzag, kde-frameworks-devel, LeGast00n, 
cblack, michaelh, ngraham, bruns

D29231: [WIP] Add keyboard_shortcuts_inhibit protocol

[Vlad Zahorodnii]

zzag requested changes to this revision.
zzag added a comment.
This revision now requires changes to proceed.


  I don't want to be selfish, but I'm not really used to the coding style in 
this patch. Could you please move method definitions outside class declarations?

INLINE COMMENTS

> keyboard_shortcuts_inhibit_interface.cpp:25
> +Private(wl_client *client, int id, SurfaceInterface *surface, 
> SeatInterface *seat, KeyboardShortcutsInhibitorInterface* q)
> +: zwp_keyboard_shortcuts_inhibitor_v1(client, id, s_version)
> +, q(q)

What if the client requested version < s_version? You need to create the 
resource manually.

> keyboard_shortcuts_inhibit_interface.cpp:129
> +Q_UNUSED(resource)
> +// TODO ensure we delete all inhibitors
> +delete q;

We probably need to ask folks in `#wayland` whether inhibitors outlive the 
manager object.

> keyboard_shortcuts_inhibit_interface.h:32
> +{
> +Q_OBJECT
> +public:

Indentation. Also, it's probably a minor thing, but it's really nice when 
Q_OBJECT is separated from code below by a single empty line (cosmetics).

> keyboard_shortcuts_inhibit_interface.h:60
> + */
> +KeyboardShortcutsInhibitorInterface 
> *getShortcutsInhibitor(SurfaceInterface *surface, SeatInterface *seat) const;
> +

We probably don't need it to be public. Only emit inhibitorCreated.

REPOSITORY
  R127 KWayland

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

To: bport, zzag, davidedmundson, apol
Cc: crossi, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D27859: [server] Expose SurfaceRole class

[Vlad Zahorodnii]

zzag abandoned this revision.
zzag added a comment.


  Given our planned kwaylandserver changes, we don't need this patch.

REPOSITORY
  R127 KWayland

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

To: zzag, #kwin
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29054: [Wayland] Add to PlasmaWindowManagement protocol windows stacking order

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> plasma-window-management.xml:74
>  
> +
> +

To be in-line with upstream standards, please add ``.

REPOSITORY
  R127 KWayland

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

To: bport, zzag, davidedmundson
Cc: meven, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D27860: [server] Add some sub-surface life cycle signals

[Vlad Zahorodnii]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:8945c1f7baaf: [server] Add some sub-surface life cycle 
signals (authored by zzag).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D27860?vs=76996=80998#toc

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27860?vs=76996=80998

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

AFFECTED FILES
  src/server/surface_interface.cpp
  src/server/surface_interface.h

To: zzag, #kwin, apol
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29054: [Wayland] Add to PlasmaWindowManagement protocol windows stacking order

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> plasmawindowmanagement_interface.cpp:297
>  
> +void PlasmaWindowManagementInterface::setStackingOrder(const 
> QVector& stackingOrder)
> +{

coding style: whitespace before `&`

REPOSITORY
  R127 KWayland

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

To: bport, zzag, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29054: [Wayland] Add to PlasmaWindowManagement protocol windows stacking order

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> plasmawindowmanagement.cpp:47
>  void windowCreated(org_kde_plasma_window *id, quint32 internalId);
> +void setStackingOrder(const QVector& ids);
>  

Coding style: whitespace before `&` and `*`

> plasmawindowmanagement_interface.cpp:297
>  
> +void PlasmaWindowManagementInterface::setStackingOrder(QVector 
> stackingOrder)
> +{

const ref

REPOSITORY
  R127 KWayland

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

To: bport, zzag, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29054: [Wayland] Add to PlasmaWindowManagement protocol windows stacking order

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> plasmawindowmanagement.cpp:242
>  
> +void PlasmaWindowManagement::Private::stackingOrderCallback(void *data, 
> org_kde_plasma_window_management *interface, wl_array *ids) {
> +auto wm = reinterpret_cast(data);

The opening brace must be on a new line.

> plasmawindowmanagement.cpp:251
> +
> +void PlasmaWindowManagement::Private::setStackingOrder(QVector ids)
> +{

Should be a const ref.

REPOSITORY
  R127 KWayland

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

To: bport, zzag, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29033: Remove duplicated code

[Vlad Zahorodnii]

zzag requested changes to this revision.
zzag added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> waylandintegration.cpp:191
>  [this] (PlasmaWindow *w) {
> -emit KWindowSystem::self()->windowAdded(w->internalId());
> -emit KWindowSystem::self()->stackingOrderChanged();

The old and the new code are not the same. With f(), we're going to emit 
windowRemoved() instead of windowAdded().

REPOSITORY
  R130 Frameworks integration plugin using KWayland

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

To: apol, #frameworks, zzag
Cc: zzag, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29036: Print meaningful warning when there is no QGuiApplication

[Vlad Zahorodnii]

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

REPOSITORY
  R278 KWindowSystem

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

To: broulik, #plasma, zzag
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28892: [autotests] Optimistic attempt to fix RemoteAccessTest reilability

[Vlad Zahorodnii]

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


  This seems super complicated.

REPOSITORY
  R127 KWayland

BRANCH
  master

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

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

D28498: [xdgoutput] Explicitly set version of server interface

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> display.h:281-288
>  /**
>   * Creates the XdgOutputManagerInterface
>   *
>   * @return the created manager
>   * @since 5.47
> + * @deprecated use the version that takes a version
>   */

Please use KWAYLAND_ENABLE_DEPRECATED_SINCE to make the compiler bark whenever 
someone uses the deprecated version of createXdgOutputManager.

REPOSITORY
  R127 KWayland

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

To: davidedmundson, #kwin
Cc: zzag, anthonyfieroni, apol, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D28498: [xdgoutput] Explicitly set version of server interface

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> apol wrote in display.h:296
> Passing an enum as const& is wrong although it doesn't make much of a 
> difference in practice.
> 
> `You can't introduce another createXdgOutputManager() because it's not 
> overloaded`. He's adding an overload, I don't understand.

> He's adding an overload, I don't understand.

Bad wording, my bad. We are allowed to overload only methods that are already 
overloaded.

From https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B

> You cannot...
> 
> For existing functions of any type:
> 
> add an overload (BC, but not SC: makes  ambiguous), adding overloads to 
> already overloaded functions is ok (any use of  already needed a cast).

REPOSITORY
  R127 KWayland

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

To: davidedmundson, #kwin
Cc: zzag, anthonyfieroni, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

D28498: [xdgoutput] Explicitly set version of server interface

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> display.h:297
> + */
> +XdgOutputManagerInterface *createXdgOutputManager(const 
> XdgOutputInterfaceVersion , QObject *parent = nullptr);
> +

You can't introduce another createXdgOutputManager() because it's not 
overloaded. You probably need to rename this method, e.g. 
createXdgOutputManager2.

REPOSITORY
  R127 KWayland

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

To: davidedmundson, #kwin
Cc: zzag, anthonyfieroni, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

D28442: Fix warnings

[Vlad Zahorodnii]

zzag accepted this revision.
zzag added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> buffer_interface.cpp:287-291
>  size.width(),
>  size.height(),
>  wl_shm_buffer_get_stride(shmBuffer),
>  imageFormat,
> +, this);

Please re-indent this code.

> xdgforeigntest.cpp:133
>  auto parentDeco = m_decoration->create(m_surface, this);
> +Q_UNUSED(parentDeco);
>  m_shellSurface = m_shell->createSurface(m_surface, this);

You don't need to put a semicolon after the Q_UNUSED macro.

REPOSITORY
  R127 KWayland

BRANCH
  master

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

To: apol, #frameworks, #kwin, zzag
Cc: zzag, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27338: Provide an initial implementation for input-method-unstable-v1

[Vlad Zahorodnii]

zzag added a comment.


  In D27338#638128 , @davidedmundson 
wrote:
  
  > We can't just make up policy changes ad-hoc on a review request to make it 
different to every other class in KWayland.
  
  
  Heh, if we really don't want to deviate from the rest of KWayland, then we 
have to use kwaylandscanner. Not having nested private classes would be the 
least thing that makes InputMethodInterface, InputMethodContextInterface, 
InputPanelInterface, and InputPanelSurfaceInterface different from any other 
class in KWayland; but I see your point. Anyway, we can live with nested 
private classes as long as people don't forget to mark them Q_DECL_HIDDEN. 
Should we maybe also start a discussion in kde-frameworks-devel on this matter?

REPOSITORY
  R127 KWayland

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

To: apol, #kwin, #frameworks
Cc: davidedmundson, zzag, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

D27338: Provide an initial implementation for input-method-unstable-v1

[Vlad Zahorodnii]

zzag added a comment.


  > If it makes you happy. But we're building with opt-in export symbols, it 
shouldn't make much of a difference.
  
  After reading some inline comments in D28295 
, I think it would be better to get rid of 
the nested private class thing. We won't need to use Q_DECL_HIDDEN and this is 
what Qt folks do. :-)

REPOSITORY
  R127 KWayland

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

To: apol, #kwin, #frameworks
Cc: zzag, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27356: Fix EWMH non-compliance for NET::{OnScreenDisplay, CriticalNotification}

[Vlad Zahorodnii]

This revision was automatically updated to reflect the committed changes.
Closed by commit R278:d3bc79da9256: Fix EWMH non-compliance for 
NET::{OnScreenDisplay,CriticalNotification} (authored by catherinez, committed 
by zzag).

REPOSITORY
  R278 KWindowSystem

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27356?vs=75581=78847

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

AFFECTED FILES
  src/platforms/xcb/netwm.cpp

To: catherinez, #kwin, davidedmundson
Cc: ngraham, davidedmundson, broulik, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, bruns

D27338: Provide an initial implementation for input-method-unstable-v1

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> inputmethod_interface.cpp:153-154
> +{
> +for (auto r : d->resourceMap())
> +d->send_commit_state(r->handle, serial);
> +}

I don't understand why we need `resourceMap()` here. 
zwp_input_method_context_v1 is not a global so I would expect to see something 
like

  void InputMethodContextInterface::sendCommitState(uint32 serial)
  {
  d->send_commit_state();
  }

> inputmethod_interface.cpp:206-210
> +void zwp_input_panel_surface_v1_destroy_resource(Resource * resource) 
> override {
> +wl_resource_destroy(resource->handle);
> +if (resourceMap().isEmpty())
> +q->deleteLater();
> +}

Only delete `q`.

REPOSITORY
  R127 KWayland

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

To: apol, #kwin, #frameworks
Cc: zzag, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27338: Provide an initial implementation for input-method-unstable-v1

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> inputmethod_interface.cpp:128
> +{
> +wl_resource_destroy(resource->handle);
> +if (resourceMap().isEmpty())

Don't call wl_resource_destroy() in foobar_destroy_resource(), it's dangerous.

  void zwp_input_method_context_v1_destroy(Resource *resource)
  {
  wl_resource_destroy(resource->handle);
  }
  
  void zwp_input_method_context_v1_destroy_resource(Resource *resource)
  {
  Q_UNUSED(resource)
  q->deleteLater();
  }

REPOSITORY
  R127 KWayland

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

To: apol, #kwin, #frameworks
Cc: zzag, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27338: Provide an initial implementation for input-method-unstable-v1

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> inputmethod_interface.cpp:128-129
> +wl_resource_destroy(resource->handle);
> +if (resourceMap().isEmpty())
> +q->deleteLater();
> +}

Destroy `q` in zwp_input_method_context_v1_destroy_resource(). In the 
destructor request, we usually want to destroy only the resource.

> inputmethod_interface.cpp:181
> +
> +class Q_DECL_HIDDEN InputPanelSurfaceInterface::Private : public 
> QtWaylandServer::zwp_input_panel_surface_v1
> +{

We need to destroy the wl_resource for zwp_input_panel_surface_v1 when the 
associated surface is destroyed.

> apol wrote in inputmethod_interface.cpp:179
> I don't know, just looked at it and it doesn't seem that useful?

zwp_input_panel_surface_v1 defines a surface role (based on weston code) so it 
should be a subclass of SurfaceRole. In long term, we want to do something like

  SurfaceRole *role = SurfaceRole::get(surface);
  if (role) {
  wl_resource_post_error(resource->handle, ZMY_SHELL_SURFACE_ERROR_ROLE,
 "Cannot reassign surface role"); // DIE!
  return;
  }

> apol wrote in inputmethod_interface.cpp:199-200
> I would rather not, otherwise when implementing private members they read 
> like local variables.

Frameworks' policies have no a single word about this so I guess it's okay(?) 
to put `m_`. I asked to drop `m_` because Qt folks seem to prefer not to put 
`m_` in private classes.

I would appreciate if you bring this topic up to the discussion in 
#kde-frameworks-devel since we're not consistent with naming in private classes.

REPOSITORY
  R127 KWayland

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

To: apol, #kwin, #frameworks
Cc: zzag, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

Re: Problems in KWayland causes by API and ABI compatibility promises

[Vlad Zahorodnii]

Hi,

On 3/23/20 4:44 PM, David Edmundson wrote:

Yeah, I feel that's what kwayland was originally going for. Seat, for
example, does a lot of dispatching and logic internally.
Then it drifted into being just wrappers.

We do need to answer that question definitively otherwise we'll be
forever stuck coding against each other.


I think that's the direction towards which we should move - a library 
with components that one could plug in to get started with writing a 
Wayland compositor.



This thread hasn't fully addressed your initial comments on ABI and API.


Yes, my concerns about ABI and API compatibility promises have been 
fully addressed unfortunately.



Moving is far far from trivial. We have plasma-framework depend on
kwayland client API. And moving just the server API would break the
client tests. We also have a released spectacle using kwayland.
It'll be really messy, plus there's any damage this could do to
distro's opinion on frameworks as a whole.


Yes, plasma-framework uses KWayland client API for Plasma::Dialog and 
shadows. It is worth to highlight that the shadows integration stuff is 
going to be ported to generic KWindowSystem shadows API in KF 5.70, so 
that's minus one. :-)



Realistically within KF5 we have two options:
   - We can start to introduce more versioned classes and see if that
helps address the problem before we make more drastic policy changes

   - We can put new protocols somewhere else and elevate them in
frameworks when they actually have users

My fear with the latter is that we'll end up over time, we'll start
forking existing protocols in wayland to fix things and ultimately end
up in an even messier fragmented state.


Yes, that's what I'm afraid of too. The problem with the former is that 
we can end up with a lot of versioned classes. API-wise this won't cause 
problems for end users, but it'll definitely make things more difficult 
internally, e.g. if we have two implementations of the xdg_wm_base 
protocol, then wrappers for the xdg-decoration or xdg-foreign protocol 
would need to deal with those versioned classes.


Cheers,
Vlad

Re: Problems in KWayland causes by API and ABI compatibility promises

[Vlad Zahorodnii]

Hi,

On 3/24/20 4:35 PM, Aleix Pol wrote:

We can decide what we want for KF6 and act accordingly. If for
example, we were to split kwayland into kwaylandclient and
kwaylandserver and the latter be in plasma, we could consider putting
new code in KWin or a shared repository.


I was also thinking about splitting KWayland into KWaylandClient and 
KWaylandServer. However, later on, I realized that this is a bad idea 
because it'll make implementing new protocols more difficult and testing
wrappers will become more difficult. In fact, I don't think that we'll 
be able to keep existing tests in the kwayland repo; we will probably 
have to drop them.



In fact, if we ever want to have such a kwayland server and are
serious about it, we'll want to be mindful about what we put there.
Much like any other framework, we only put things on the framework
when it's going to be shared rather than putting some parts of the
code somewhere by policy like we've been doing here.


The problem is that we can't keep KWayland stable; at least, right now. 
There are still things in the wild that need a protocol or something and 
as we implement those protocols, we might need some of existing wrappers 
to adapt to the new changes or rework them completely. One such example 
that comes to my mind is the explicit synchronization protocol.


I think it's a good thing to have helper classes for Wayland protocols 
that you could plug in and get a running compositor, this is a great 
idea, but I think it's way too early to promise API and ABI guarantees.


Cheers,
Vlad

Re: Problems in KWayland causes by API and ABI compatibility promises

[Vlad Zahorodnii]

Hi,

On 3/24/20 6:55 PM, David Edmundson wrote:

I just had a near miss with XdgOutputV1(version2) which luckily we
were able to easily guard. Had the spec claimed that in version2
sending xdg_output.name was mandatory we would have been completely
screwed.


Unfortunately, I have bad news. We're screwed. I poked people to clarify 
the language that is used in many specs. Here's the summary: if a spec 
says that an event is sent after creating an object and nowhere it says 
that the event is optional, then the compositor _must_ send the event.


xdg_output::description is an optional event because the spec mentions 
it. On the other hand, xdg_output::name is mandatory because the spec 
has no a single word about it being optional.


Cheers,
Vlad

D27338: Provide an initial implementation for input-method-unstable-v1

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> display.cpp:49
> +#include "inputmethod_interface.h"
> +
>  

Stray new line. Please remove it.

> inputmethod_interface.cpp:25
> +
> +class InputMethodContextInterface::Private : public 
> QtWaylandServer::zwp_input_method_context_v1
> +{

Add Q_DECL_HIDDEN.

> inputmethod_interface.cpp:125
> +
> +void 
> zwp_input_method_context_v1_destroy(QtWaylandServer::zwp_input_method_context_v1::Resource
>  *resource) override
> +{

In the destructor request, we need to destroy the wl_resource with 
`wl_resource_destroy()`. When the resource is destroyed and it isn't inert, 
destroy `q`.

---

We probably need to destroy the wl_resource resource here.

  wl_resource_destroy(resource->handle);

> inputmethod_interface.cpp:145-146
> +{
> +for (auto r : d->resourceMap())
> +d->send_commit_state(r->handle, serial);
> +}

I also prefer not to put braces around for's and if's when the body contains 
only one statement. Code looks more compact; but we follow the Frameworks 
coding style and should put braces even if it's a single statement.

> inputmethod_interface.cpp:179
> +
> +class InputPanelSurfaceInterface::Private : public 
> QtWaylandServer::zwp_input_panel_surface_v1
> +{

Shouldn't `InputPanelSurfaceInterface` be also a subclass of `SurfaceRole`?

> inputmethod_interface.cpp:199-200
> +InputPanelSurfaceInterface *const q;
> +QPointer m_surface;
> +bool m_overlay = false;
> +};

Naming nitpick: in `FooPrivate` classes, we avoid putting `m_`.

> inputmethod_interface.cpp:220
> +
> +void zwp_input_panel_v1_get_input_panel_surface(Resource *resource, 
> uint32_t id, struct ::wl_resource *surface) override
> +{

Naming nitpick: it would be nice to avoid names such as `surfaceIface` or 
`surfaceInterface`. I would like to highlight that `surfaceIface` is a bad name 
according to the Frameworks coding style.

Suggestion: rename `surface` to `surfaceResource` and then do `auto surface = 
SurfaceInterface::get(surfaceResource);`.

> inputmethod_interface.cpp:224
> +
> +auto ipsi = new InputPanelSurfaceInterface(nullptr);
> +ipsi->d->init(resource->client(), id, resource->version());

No abbreviations.

> inputmethod_interface.h:13
> +
> +#include "resource.h"
> +

Do we actually need it?

> inputmethod_interface.h:102
> +Q_SIGNALS:
> +void inputPanelSurfaceAdded(quint32 id, InputPanelSurfaceInterface 
> *surface);
> +

API design nitpick: it would be nice to have a signal with only parameter - 
InputPanelSurfaceInterface *surface. Is there a reason why 
InputPanelSurfaceInterface can't have an id getter?

REPOSITORY
  R127 KWayland

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

To: apol, #kwin, #frameworks
Cc: zzag, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28245: [xdgoutput] Only send initial name and description if set

[Vlad Zahorodnii]

zzag added a comment.


  In D28245#633649 , @davidedmundson 
wrote:
  
  > I interpreted the spec the same way when I wrote this...but we don't have a 
choice.
  
  
  I think we do.
  
  (a) Revert the recent xdg-output patches and land them in KF 5.70 or later. 
(Plasma 5.19 is going to depend on KF 5.70)
  
  (b) Introduce an API to indicate what version of the protocol is actually 
implemented by the compositor.
  
m_xdgOutputManager = m_display->createXdgOutputManager(m_display);
m_xdgOutputManager->setVersion(XdgOutputManagerV1Interface::V2);
m_xdgOutputManager->create();
  
  I think it's okay to ship the current fix, but only temporarily.
  
  >> I believe it might be a problem
  > 
  > From a QtWayland POV it's fine at least.
  > 
  >> We should remove these ifs in KF 5.70.
  > 
  > Why then?
  > 
  > Removing them then would still break Plasma 5.18 and officially we support 
that combo
  > 
  > Plasma 5.19 will set the name/description, so removing the if won't change 
anything.

REPOSITORY
  R127 KWayland

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

To: davidedmundson, #kwin, zzag, apol
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28245: [xdgoutput] Only send initial name and description if set

[Vlad Zahorodnii]

zzag added a comment.


  The spec says that `name` and `description` events will be sent after the 
`xdg-output` is created. I believe it might be a problem. We should remove 
these `if`s in KF 5.70.

REPOSITORY
  R127 KWayland

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

To: davidedmundson, #kwin, zzag, apol
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28245: [xdgoutput] Only send initial name and description if set

[Vlad Zahorodnii]

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

REPOSITORY
  R127 KWayland

BRANCH
  master

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

To: davidedmundson, #kwin, zzag
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28168: Broadcast application menu to resources when registering them

[Vlad Zahorodnii]

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

REPOSITORY
  R127 KWayland

BRANCH
  cblack/broadcast-appmenu-on-resource-register (branched from master)

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

To: cblack, #kwin, zzag
Cc: kde-frameworks-devel, zzag, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28164: Add test for application menu in PWM interface

[Vlad Zahorodnii]

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

REPOSITORY
  R127 KWayland

BRANCH
  cblack/appmenu-tests (branched from master)

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

To: cblack, #kwin, zzag
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28147: [server] Don't make assumptions about the order of damage_buffer and attach requests

[Vlad Zahorodnii]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:ff9cadf00f21: [server] Dont make assumptions about 
the order of damage_buffer and attach… (authored by zzag).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28147?vs=78041=78044

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

AFFECTED FILES
  src/server/surface_interface.cpp

To: zzag, #kwin, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D28147: [server] Don't make assumptions about the order of damage_buffer and attach requests

[Vlad Zahorodnii]

zzag added a comment.


  In general, the idea of raising a protocol error when the client tries to 
damage a surface with no buffer seems sensible. But the spec doesn't say 
damage_buffer requests must be followed by attach requests and we can't really 
predict the future.

REPOSITORY
  R127 KWayland

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

To: zzag, #kwin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28147: [server] Don't make assumptions about the order of damage_buffer and attach requests

[Vlad Zahorodnii]

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

REVISION SUMMARY
  The spec says nothing about the order between damage_buffer and attach
  requests.

TEST PLAN
  Firefox doesn't become frozen. Although there are still issues with resizing.

REPOSITORY
  R127 KWayland

BRANCH
  dont-assume-that-damage-buffer-must-be-called-before-attach

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

AFFECTED FILES
  src/server/surface_interface.cpp

To: zzag, #kwin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D14910: Pass a dedicated fd to each keyboard for the xkb keymap

[Vlad Zahorodnii]

zzag added a comment.


  Urgh, the authorship information is screwed up. :/

REPOSITORY
  R127 KWayland

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

To: graesslin, #kwin, #frameworks, davidedmundson, zzag
Cc: plasma-devel, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D14910: Pass a dedicated fd to each keyboard for the xkb keymap

[Vlad Zahorodnii]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:6bfa71d89aee: Pass a dedicated fd to each keyboard for 
the xkb  keymap (authored by zzag).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14910?vs=77916=78000

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

AFFECTED FILES
  autotests/client/test_wayland_seat.cpp
  src/server/CMakeLists.txt
  src/server/keyboard_interface.cpp
  src/server/keyboard_interface.h
  src/server/keyboard_interface_p.h
  src/server/seat_interface.cpp
  src/server/seat_interface.h
  src/server/seat_interface_p.h

To: graesslin, #kwin, #frameworks, davidedmundson, zzag
Cc: plasma-devel, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D28016: KWindowSystem: deprecate KStartupInfoData::launchedBy, unused

[Vlad Zahorodnii]

zzag added a comment.


  Maybe it would be worth to send an email to wm-spec-list proposing to 
deprecate LAUNCHED_BY, but on the other hand fixing X11 stuff in any way is 
like beating a dead horse.

REPOSITORY
  R278 KWindowSystem

BRANCH
  master

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

To: dfaure, zzag, broulik, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28112: Expose application menu via KWindowInfo

[Vlad Zahorodnii]

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

REPOSITORY
  R278 KWindowSystem

BRANCH
  appmenu-x11 (branched from master)

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

To: cblack, #plasma, broulik, zzag, #kwin
Cc: zzag, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27828: [server] Introduce SurfaceInterface::boundingRect()

[Vlad Zahorodnii]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:deb476e47d47: [server] Introduce 
SurfaceInterface::boundingRect() (authored by zzag).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27828?vs=76922=77918

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

AFFECTED FILES
  src/server/surface_interface.cpp
  src/server/surface_interface.h

To: zzag, #kwin, davidedmundson
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28112: [WIP] Expose application menu via KWindowInfo

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> netwm.cpp:3554
> +
> +const char* NETWinInfo::appMenuObjectPath() const
> +{

nit: add whitespace before `*`

> netwm.cpp:3559
> +
> +const char* NETWinInfo::appMenuServiceName() const
> +{

ditto

REPOSITORY
  R278 KWindowSystem

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

To: cblack, #plasma, broulik, zzag, #kwin
Cc: zzag, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D14910: Pass a dedicated fd to each keyboard for the xkb keymap

[Vlad Zahorodnii]

zzag updated this revision to Diff 77916.
zzag added a comment.


  Rebase on master

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14910?vs=39945=77916

BRANCH
  arcpatch-D14910

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

AFFECTED FILES
  autotests/client/test_wayland_seat.cpp
  src/server/CMakeLists.txt
  src/server/keyboard_interface.cpp
  src/server/keyboard_interface.h
  src/server/keyboard_interface_p.h
  src/server/seat_interface.cpp
  src/server/seat_interface.h
  src/server/seat_interface_p.h

To: graesslin, #kwin, #frameworks, davidedmundson, zzag
Cc: plasma-devel, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

Re: Problems in KWayland causes by API and ABI compatibility promises

[Vlad Zahorodnii]

On 3/17/20 12:27 PM, David Edmundson wrote:

IMHO we're lacking a "what actually is kwayland?"  and an accurate
definition of what's the added value compared to just using the auto
generated classes directly.


That's a good question! On one hand, it's nice to have Qt-friendly 
wrappers for Wayland protocols. On the other hand, it would be even 
better to have a library to write Wayland compositors. So, one just 
needs to plug a few classes in order to get started with drm and 
xdg-shell stuff.





To bootstrap this I've started an initial list of discrete yes/no
questions to help serve as a starting point of what kwayland's
direction specifically should be.

  - Is it kwayland's job to abstract different versions of the same protocol?


Yes.


  - Is it kwayland's job to abstract different protocols that are
semantically similar?
(including xdgshellv6 and wm_base)


No.


- Is it kwayland's job to turn an event-driven API into a property-driven API?
(i.e turning the request set_minimum_size into an API where you can
query what was last received)


Yes.


  - Is it kwayland's job to abstract receiving double buffered stuff?
(i.e the getter above only gets the value once committed)


That's a difficult question. I can't answer it right now.


  - Is it kwayland's job to abstract sending double buffered stuff?
(i.e implicitly send "wl_output.done() after Output::setSize())


Probably, not.


  - Is it kwayland's job to be a multiplexer?
(i.e updating xdgoutput or plasmawindowmanagement forwards events to
all listening resources)
If so should we express this difference in the API or have them as
methods on the global? Same for the sending to only the focussed
window?


Probably, yes. I'm not sure about the API part, though.


  - Is it kwayland's job to convert basic types into Qt friendly types?


Yes.

It's worth to point out that many wayland protocols are still being 
actively developed. So, some API design choices must be adjusted as we 
implement Wayland protocols.


Cheers,
vlad

D28112: [WIP] Expose application menu via KWindowInfo

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> netwm.cpp:4697-4707
>  if (dirty2 & WM2GTKFrameExtents) {
>  p->gtk_frame_extents = NETStrut();
>  
>  QVector data = get_array_reply(p->conn, 
> cookies[c++], XCB_ATOM_CARDINAL);
>  if (data.count() == 4) {
>  p->gtk_frame_extents.left   = data[0];
>  p->gtk_frame_extents.right  = data[1];

Please move WM2GTKFrameExtents above WM2AppMenuObjectPath to match the order in 
which `cookies` is filled.

REPOSITORY
  R278 KWindowSystem

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

To: cblack, #plasma, broulik, zzag, #kwin
Cc: zzag, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28112: [WIP] Expose application menu via KWindowInfo

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> zzag wrote in netwininfotestclient.cpp:281-319
> You have to change properties via xcb_change_property().
> 
>   void NetWinInfoTestClient::testAppMenuObjectPath()
>   {
>   ATOM(_KDE_NET_WM_APPMENU_OBJECT_PATH)
>   INFO
>   
>   QVERIFY(!info.appMenuObjectPath());
>   
>   xcb_change_property(connection(), XCB_PROP_MODE_REPLACE, m_testWindow,
>   atom, XCB_ATOM_STRING, 8, 4, "foo\0");
>   xcb_flush(connection());
>   
>   waitForPropertyChange(, atom, NET::Property(0), 
> NET::WM2AppMenuObjectPath);
>   QCOMPARE(info.appMenuObjectPath(), "foo");
>   }
>   
>   void NetWinInfoTestClient::testAppMenuServiceName()
>   {
>   ATOM(_KDE_NET_WM_APPMENU_SERVICE_NAME)
>   INFO
>   
>   QVERIFY(!info.appMenuServiceName());
>   
>   xcb_change_property(connection(), XCB_PROP_MODE_REPLACE, m_testWindow,
>   atom, XCB_ATOM_STRING, 8, 4, "foo\0");
>   xcb_flush(connection());
>   
>   waitForPropertyChange(, atom, NET::Property(0), 
> NET::WM2AppMenuServiceName);
>   QCOMPARE(info.appMenuServiceName(), "foo");
>   }

please get rid of '\0'

REPOSITORY
  R278 KWindowSystem

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

To: cblack, #plasma, broulik, zzag, #kwin
Cc: zzag, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28112: [WIP] Expose application menu via KWindowInfo

[Vlad Zahorodnii]

zzag requested changes to this revision.
zzag added a reviewer: KWin.
zzag added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> netwininfotestclient.cpp:281-319
> +void NetWinInfoTestClient::testAppMenuObjectPath()
> +{
> +ATOM(_KDE_NET_WM_APPMENU_OBJECT_PATH)
> +UTF8
> +INFO
> +
> +QVERIFY(!info.appMenuObjectPath());

You have to change properties via xcb_change_property().

  void NetWinInfoTestClient::testAppMenuObjectPath()
  {
  ATOM(_KDE_NET_WM_APPMENU_OBJECT_PATH)
  INFO
  
  QVERIFY(!info.appMenuObjectPath());
  
  xcb_change_property(connection(), XCB_PROP_MODE_REPLACE, m_testWindow,
  atom, XCB_ATOM_STRING, 8, 4, "foo\0");
  xcb_flush(connection());
  
  waitForPropertyChange(, atom, NET::Property(0), 
NET::WM2AppMenuObjectPath);
  QCOMPARE(info.appMenuObjectPath(), "foo");
  }
  
  void NetWinInfoTestClient::testAppMenuServiceName()
  {
  ATOM(_KDE_NET_WM_APPMENU_SERVICE_NAME)
  INFO
  
  QVERIFY(!info.appMenuServiceName());
  
  xcb_change_property(connection(), XCB_PROP_MODE_REPLACE, m_testWindow,
  atom, XCB_ATOM_STRING, 8, 4, "foo\0");
  xcb_flush(connection());
  
  waitForPropertyChange(, atom, NET::Property(0), 
NET::WM2AppMenuServiceName);
  QCOMPARE(info.appMenuServiceName(), "foo");
  }

> netwm_def.h:727-728
>  WM2GTKFrameExtents = 1u << 27, // NOT STANDARD @since 5.65
> +WM2AppMenuServiceName  = 1u << 28,
> +WM2AppMenuObjectPath   = 1u << 29,
>  WM2AllProperties   = ~0u

Missing `// NOT STANDARD @since`

> netwm.cpp:3536
> +xcb_change_property(p->conn, XCB_PROP_MODE_REPLACE, p->window, 
> p->atom(_KDE_NET_WM_APPMENU_OBJECT_PATH),
> +p->atom(UTF8_STRING), 8, 
> strlen(p->appmenu_object_path),
> +(const void *) p->appmenu_object_path);

s/p->atom(UTF8_STRING)/XCB_ATOM_STRING/

> netwm.cpp:3550
> +xcb_change_property(p->conn, XCB_PROP_MODE_REPLACE, p->window, 
> p->atom(_KDE_NET_WM_APPMENU_SERVICE_NAME),
> +p->atom(UTF8_STRING), 8, 
> strlen(p->appmenu_service_name),
> +(const void *) p->appmenu_service_name);

s/p->atom(UTF8_STRING)/XCB_ATOM_STRING/

> netwm.cpp:4681
> +
> +const QByteArray id = get_string_reply(p->conn, cookies[c++], 
> p->atom(UTF8_STRING));
> +if (id.length() > 0) {

s/p->atom(UTF8_STRING)/XCB_ATOM_STRING/

> netwm.cpp:4691
> +
> +const QByteArray id = get_string_reply(p->conn, cookies[c++], 
> p->atom(UTF8_STRING));
> +if (id.length() > 0) {

s/p->atom(UTF8_STRING)/XCB_ATOM_STRING/

REPOSITORY
  R278 KWindowSystem

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

To: cblack, #plasma, broulik, zzag, #kwin
Cc: zzag, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D26858: Provide an implementation for the tablet interface

[Vlad Zahorodnii]

zzag added inline comments.

INLINE COMMENTS

> test_tablet_interface.cpp:1-19
> +/
> +Copyright 2020 Aleix Pol Gonzalez 
> +
> +This library is free software; you can redistribute it and/or
> +modify it under the terms of the GNU Lesser General Public
> +License as published by the Free Software Foundation; either
> +version 2.1 of the License, or (at your option) version 3, or any

Note that we've switched to SPDX markers.

REPOSITORY
  R127 KWayland

BRANCH
  arcpatch-D26858

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

To: apol, #kwin, #frameworks, zzag
Cc: davidedmundson, zzag, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

D28056: KWindowSystem: Convert license headers to SPDX expressions

[Vlad Zahorodnii]

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

REPOSITORY
  R278 KWindowSystem

BRANCH
  spdx

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

To: cordlandwehr, zzag
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28058: KWayland: Convert license headers to SPDX

[Vlad Zahorodnii]

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

REPOSITORY
  R127 KWayland

BRANCH
  spdx

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

To: cordlandwehr, zzag
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28016: KWindowSystem: deprecate KStartupInfoData::launchedBy, unused

[Vlad Zahorodnii]

zzag added a comment.


  Do you know why LAUNCHED_BY was added? The spec [1] has no a single word 
about its potential use cases.
  
  [1] 
https://specifications.freedesktop.org/startup-notification-spec/startup-notification-latest.txt

REPOSITORY
  R278 KWindowSystem

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

To: dfaure, zzag, broulik, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27464: Add application menu dbus paths to org_kde_plasma_window interface

[Vlad Zahorodnii]

zzag added a comment.


  In D27464#625403 , @broulik wrote:
  
  > Didn't we have a dedicated protocol for window menu in plasma-integration? 
Or is this just for reading? We surely don't want all apps to use plasma 
surface interface for announcing the global menu?
  
  
  Okay, let's step back. We have a proprietary protocol that is used to 
announce application menus. The problem is that only kwin knows that a 
particular window has that particular application menu. We need a way to share 
this information with a global menu applet.
  
  > We surely don't want all apps to use plasma surface interface for 
announcing the global menu?
  
  Could you please explain why we need to announce global menus?

REPOSITORY
  R127 KWayland

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

To: cblack, #kwin, zzag, davidedmundson
Cc: broulik, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

D27464: Add application menu dbus paths to org_kde_plasma_window interface

[Vlad Zahorodnii]

zzag added a comment.


  In D27464#625403 , @broulik wrote:
  
  > Didn't we have a dedicated protocol for window menu in plasma-integration?
  
  
  That protocol is for announcing global menus.
  
  > Or is this just for reading?
  
  Yes, it's for reading.
  
  > We surely don't want all apps to use plasma surface interface for 
announcing the global menu?
  
  Could you please clarify what you mean by "plasma surface interface"?

REPOSITORY
  R127 KWayland

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

To: cblack, #kwin, zzag, davidedmundson
Cc: broulik, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

Problems in KWayland causes by API and ABI compatibility promises

[Vlad Zahorodnii]

Hi,

KWayland is a library that provides convenience wrappers for Wayland 
protocols. Usually, when we want to implement some Wayland protocol, 
first, we add corresponding wrappers in KWayland and after that we 
actually implement the protocol in KWin.


Unfortunately, we made a bad design choice in KWayland. It is common in 
KWayland to see abstractions over unstable protocols. We do it in the 
name of "don't repeat yourself" principle. This reduces the amount of 
duplicated code in KWin, but it also makes much more difficult to extend 
KWayland. Abstracting unstable protocols turns out to be really bad when 
the next version of an unstable protocol contains big changes, e.g. 
xdg-shell-unstable-v5 -> xdg-shell-unstable-v6. Due to the API and ABI 
compatibility promises, we can't fix the broken design of those 
wrappers. We should not try to abstract over Wayland protocols in the 
first place!


We need to fix those wrappers in order to implement the corresponding 
protocols properly in KWin (and fix a few bugs).


One option is to make KWayland unstable so we are free to do necessary 
changes to fix the wrappers, but it doesn't really go along with the 
Frameworks' policies. Another option is to move KWayland somewhere else, 
e.g. KWin or Plasma.


Alternatively, we could provide new wrappers and append a suffix to 
class names, e.g. "2" or "V2". But I don't like this approach for a 
couple of reasons. First, we are going to mix two types of versions in a 
single class name, e.g. XdgToplevelV6InterfaceV2, etc. Second, some 
protocols are not completely implemented in KWin, so we may still need 
to change wrappers in a way that may break API or ABI compatibility. 
Arguably, we just don't have to push semi-finished changes, but 
implementing everything all at once is a bit difficult and tackling one 
problem at a time is much easier.


I'm writing this email to bring problems that we currently have in 
KWayland up to discussion and find the way in which we can address them.


For what it's worth the question of moving KWayland out of Frameworks to 
somewhere else had been raised before, but the discussion got stalled... 
sort of. [1]


Cheers,
Vlad

[1] https://phabricator.kde.org/T11903

D27859: [server] Expose SurfaceRole class

[Vlad Zahorodnii]

zzag added a dependent revision: D27860: [server] Add some sub-surface life 
cycle signals.

REPOSITORY
  R127 KWayland

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

To: zzag, #kwin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27828: [server] Introduce SurfaceInterface::boundingRect()

[Vlad Zahorodnii]

zzag added a dependent revision: D27859: [server] Expose SurfaceRole class.

REPOSITORY
  R127 KWayland

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

To: zzag, #kwin, davidedmundson
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27860: [server] Add some sub-surface life cycle signals

[Vlad Zahorodnii]

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

REVISION SUMMARY
  These signals can be very useful when one wants to monitor changes in a
  sub-surface tree.

REPOSITORY
  R127 KWayland

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

AFFECTED FILES
  src/server/surface_interface.cpp
  src/server/surface_interface.h

To: zzag, #kwin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27860: [server] Add some sub-surface life cycle signals

[Vlad Zahorodnii]

zzag added a dependency: D27859: [server] Expose SurfaceRole class.

REPOSITORY
  R127 KWayland

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

To: zzag, #kwin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27859: [server] Expose SurfaceRole class

[Vlad Zahorodnii]

zzag added a dependency: D27828: [server] Introduce 
SurfaceInterface::boundingRect().

REPOSITORY
  R127 KWayland

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

To: zzag, #kwin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27859: [server] Expose SurfaceRole class

[Vlad Zahorodnii]

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

REVISION SUMMARY
  This can be useful for implementing out-of-tree shell surface protocols.

REPOSITORY
  R127 KWayland

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

AFFECTED FILES
  src/server/CMakeLists.txt
  src/server/generic_shell_surface_p.h
  src/server/surface_interface.cpp
  src/server/surfacerole.cpp
  src/server/surfacerole.h
  src/server/surfacerole_p.h

To: zzag, #kwin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27828: [server] Introduce SurfaceInterface::boundingRect()

[Vlad Zahorodnii]

zzag added a comment.


  I'll merge this patch after 5.68 is tagged.

REPOSITORY
  R127 KWayland

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

To: zzag, #kwin, davidedmundson
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27828: [server] Introduce SurfaceInterface::boundingRect()

[Vlad Zahorodnii]

zzag added a comment.


  In D27828#621821 , @apol wrote:
  
  > Maybe producing the patch that is meant to consume this will help see 
what's the use for this patch.
  
  
  D27831 

REPOSITORY
  R127 KWayland

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

To: zzag, #kwin
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27828: [server] Introduce SurfaceInterface::boundingRect()

[Vlad Zahorodnii]

zzag updated this revision to Diff 76922.
zzag added a comment.


  Add missing @since

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27828?vs=76921=76922

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

AFFECTED FILES
  src/server/surface_interface.cpp
  src/server/surface_interface.h

To: zzag, #kwin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns