[Bug] Screenshot with KDE and Wayland

[letsfindaway]

I just started testing the current dev regarding the Wayland features. I have seen that you had to add some changes for GNOME, but those now don't work well with KDE.

The points I have are:

• When you set the interactive=true option, a dialog is shown when requesting a screenshot in Desktop mode. On KDE, this dialog is quite limited (and look ugly): You can just select three options:

  • Fullscreen (means all screens)
  • Current screen
  • Current window

  and you can select to include or exclude the mouse cursor, but you have no option to select an arbitrary area.

• Additionally the screenshot contains the KDE dialog. This might be an KDE error, but of course it is not helpful.

So I think for KDE I have to use the non-interactive mode (which always captures all screens) and need the UBCustomCaptureWindow to select the area.

It is really bad that the application has to handle different behaviors of GNOME, KDE and probably other desktop environments, but this is how Wayland behaves: You have an API (the desktop portal), but you cannot predict what happens when you invoke it. Sigh.

I'm quite sure we will come across similar things in the future. So should we introduce a common piece of code to determine the desktop environment? There is the XDG_CURRENT_DESKTOP environment variable which should tell us.

@kaamui: Comments? More ideas?

[letsfindaway]

If we need to distinguish between DEs, then I would like to have such case differentiation only in classes which are compiled for Linux only. There should be no if (GNOME) in any class compiled for Windows or MacOS.

But I also don't want it in UBDesktopPortal. This should be the wrapper around the portal functions only.

If we look at the currently implemented GNOME way, then we see a completely different solution for selecting the screenshot area:

• On GNOME, the area can be selected by the user as part of the operating taking the screenshot.
• On all other platforms a screenshot of the full screen is taken, and UBCustomCaptureWindow is used to select part of it.

To handle both cases without asking for WAYLAND or GNOME in the UBDesktopAnnotationController, we could extend the API for UBDisplayManager::grab() and add a flag telling that we only need a part of the screen. Platforms supporting this return that part together with a flag telling that they already managed the selection. Others return the complete screen. The caller can then decide whether it needs UBCustomCaptureWindow.

On the other end, i.e. in UBPlatformUtils::grabScreen(), we can then do the DE specific handling and call the UBDesktopPortal::grabScreen() with a flag whether it should be interactive or not.

[kaamui]

I just started testing the current dev regarding the Wayland features. I have seen that you had to add some changes for GNOME, but those now don't work well with KDE.

Sorry about that, I was not able to test the impact on KDE at the time, and knew there was a risk to break compatibility on it. I'll find time to install KDE to be able to test the features on both envs.

On the Gnome-side, the screen capture of the OS is quite good :

OpenBoard.Cast.23.mp4

One change I'll need to do on Gnome is to no longer have two buttons, because as you can see the OS screen capture app comes directly with its own separated buttons, so it's redundant.

The points I have are:

• When you set the interactive=true option, a dialog is shown when requesting a screenshot in Desktop mode. On KDE, this dialog is quite limited (and look ugly): You can just select three options:

  • Fullscreen (means all screens)
  • Current screen
  • Current window

That's bad news. I was hoping the screen capture feature coming with KDE would be quite similar to the one in the video above.

and you can select to include or exclude the mouse cursor, but you have no option to select an arbitrary area.

• Additionally the screenshot contains the KDE dialog. This might be an KDE error, but of course it is not helpful.

:/

So I think for KDE I have to use the non-interactive mode (which always captures all screens) and need the UBCustomCaptureWindow to select the area.

It is really bad that the application has to handle different behaviors of GNOME, KDE and probably other desktop environments, but this is how Wayland behaves: You have an API (the desktop portal), but you cannot predict what happens when you invoke it. Sigh.

Yep, I don't think we will have another choice, for the next couple years

I'm quite sure we will come across similar things in the future. So should we introduce a common piece of code to determine the desktop environment? There is the XDG_CURRENT_DESKTOP environment variable which should tell us.

@kaamui: Comments? More ideas?

Totally agree. We need to do our best to keep the code as identical between KDE and Gnome as possible, and to only introduce some env specific changes when mandatory.

[letsfindaway]

We need to do our best to keep the code as identical between KDE and Gnome as possible, and to only introduce some env specific changes when mandatory.

Thanks for the explanations. My current idea is to add a function bool grabCanCrop() to the UBPlatformUtils. The function returns true, if UBPlatformUtils::grabScreen() already can handle cropping. This can be used to hide the second button, and also to answer the question whether UBCustomCaptureWindow is needed or not. It also avoids that we have any if (GNOME) outside of the platform utils.

In UBPlatformUtils::grabScreen() and UBDisplayManager::grab(), the rect parameter will be removed, as it is never used. Instead we add a flag telling whether user cropping is requested or not.

We also remove the rect parameter in UBDesktopPortal::grabScreen() and add a flag requesting interactive mode. So we even keep an if (GNOME) out of the UBDesktopPortal.

[kaamui]

If we need to distinguish between DEs, then I would like to have such case differentiation only in classes which are compiled for Linux only. There should be no if (GNOME) in any class compiled for Windows or MacOS.

But I also don't want it in UBDesktopPortal. This should be the wrapper around the portal functions only.

If we look at the currently implemented GNOME way, then we see a completely different solution for selecting the screenshot area:

• On GNOME, the area can be selected by the user as part of the operating taking the screenshot.
• On all other platforms a screenshot of the full screen is taken, and UBCustomCaptureWindow is used to select part of it.

To handle both cases without asking for WAYLAND or GNOME in the UBDesktopAnnotationController, we could extend the API for UBDisplayManager::grab() and add a flag telling that we only need a part of the screen. Platforms supporting this return that part together with a flag telling that they already managed the selection. Others return the complete screen. The caller can then decide whether it needs UBCustomCaptureWindow.

On the other end, i.e. in UBPlatformUtils::grabScreen(), we can then do the DE specific handling and call the UBDesktopPortal::grabScreen() with a flag whether it should be interactive or not.

We need to agree on the responsibilities of each class. What UBDesktopPortal is responsible for, and what about UBPlatformUtils essentially ?

If I understand well, the current name of UBDesktopPortal is to reflect its link to xdg-desktop-portal. From the doc : "Portals were designed for use with applications sandboxed through Flatpak, but any application can use portals to provide uniform access to features independent of desktops and toolkits."

Reading that seems to confirm your initial take, that UBDesktopPortal should not care about GNOME/KDE.

As a side note, UBDesktopPortal's name doesn't reflect much its responsibility. If we ask ourselves what is its responsibilty, its name should be the answer to this question. Also, It's maybe the first class that is totally Linux-specific, so maybe it should be moved in a "_linux" file ? I don't know.

My attempt to define its responsibility :

• UBDesktopPortal is responsible for managing screen casting on Linux.

So maybe it should be named UBLinuxScreenCastManager ?

[letsfindaway]

We need to agree on the responsibilities of each class. What UBDesktopPortal is responsible for, and what about UBPlatformUtils essentially ?

If I understand well, the current name of UBDesktopPortal is to reflect its link to xdg-desktop-portal. From the doc : "Portals were designed for use with applications sandboxed through Flatpak, but any application can use portals to provide uniform access to features independent of desktops and toolkits."

Reading that seems to confirm your initial take, that UBDesktopPortal should not care about GNOME/KDE.

Yes, this is my understanding and intention.

As a side note, UBDesktopPortal's name doesn't reflect much its responsibility. If we ask ourselves what is its responsibilty, its name should be the answer to this question.

IMHO its only responsibility is to provide a C++/Qt wrapper around the DBus interface of some of the portal functions.

Also, It's maybe the first class that is totally Linux-specific, so maybe it should be moved in a "_linux" file ? I don't know.

Then the same would apply to UBPipewireSink, of course. I'm not a fan of reflecting this in a file name and, as a consequence, the class name. Another option would be to move such files to a linux subdirectory, similar as we do it for podcast.

My attempt to define its responsibility :

• UBDesktopPortal is responsible for managing screen casting on Linux.

No, this is not what it should do. It currently provides a wrapper around the functions of the Screenshot portal and the ScreenCast portal and executes individual API calls in sequence to provide the overall function of the portal. If we later find the necessity to use another portal, then it should also go here.

So maybe it should be named UBLinuxScreenCastManager ?

It is not only about ScreenCast and it is not a Manager. I think the current name is not so bad. You might append Wrapper if you like.

[kaamui]

I'd like to avoid confusion. I don't like the approach of naming objects from a technical point of view, instead of reflecting "business" intention. When a dev is going to search for code related to screen capture, it will search for "screen capture" not for "desktop portal". And I don't like abreviations either for the same reasons. You'll search for desktopEnvironment, not for de (it's just to illustrate, in this case it is actually no big deal because in the scope of one simple function). You don't want other devs to have to dig into your classes and their usage to understand what it is all about

I personnly think ÙBDesktopPortal name is terrible 😄 because it's confusing and not other-devs-friendly. We must take care of the developer experience (DX), especially on an opensource project. If you ask 10 devs to tell what this class does actually in the code, how many will be able to tell without looking into it that it handles screen capture on Gnome and KDE ? And I don't like either the "Linux" in the name, for the reasons mentionned above, but it must be clear in some way that it is linux specific. when we don't have the choice to have non-portable code.

And if we go for more interfaces to be handled in UBDesktopPortal, then its responsibility should be to manage the interfaces and that's all (SRP), and all the interfaces should have their own dedicated class (to respect ISP),

In the following table : https://wiki.archlinux.org/title/XDG_Desktop_Portal, if we decide we want to interface with the Print portal tomorrow, then UBDesktopPortal class won't have to handle the print logic. We'll have a dedicated class, responsible for printing on linux using this xdg-desktop-portal interface, and ÙBDesktopPortal will be there maybe only to handle the DBus communication of the different interfaces, including this one, or just to instanciate them and manage them (just an example, probably not exactly what we should do)

So I can agree that UBLinuxScreenCastManager is not a good solution, if we look at the bigger picture, but still, UBDesktopPortal is not improving DX.

We can then find simple solutions that won't be perfect but at least help a bit, or go deep in the architecture and rework how we think this part of the code. Simple solution would be to rename it UBXDGDesktopPortalWrapper or moving it to a linux folder and rename it UBDesktopPortalWrapper. Option 3 would be to continue to discuss at the conception level, and see where it ends.

Option 3 is probably what will take the most time but provide the best benefits to OpenBoard, option 2 is OK for me, even option 1 could be OK.

Also, it does not have to be solved today. I just took the chance of this discussion to put the subject on the table.

What do you think ?

[letsfindaway]

I'd like to avoid confusion. I don't like the approach of naming objects from a technical point of view, instead of reflecting "business" intention. When a dev is going to search for code related to screen capture, it will search for "screen capture" not for "desktop portal". And I don't like abreviations either for the same reasons. You'll search for desktopEnvironment, not for de (it's just to illustrate, in this case it is actually no big deal because in the scope of one simple function). You don't want other devs to have to dig into your classes and their usage to understand what it is all about

I think we had similar discussions in the past ;) I always like good discussions. They help me thinking about the reasoning behind my opinions.

I think not each class has a "business" and there are classes, where a name reflecting their technical part is good. Look at the following examples:

• Classes reflecting entities, e.g. QFile, QScreen, UBDocument, ...
• Classes for technical tasks, e.g. QXmlReader, UBStringUtils, ...

Such class names tell, what they do, but they do not tell for what business they are used in a specific application. And I think all classes in the framework folder are classes which don't have just one specific business, even if they are focused on a specific task or set of tasks, like UBPlatformUtils (platform abstraction) or UBPipewireSink (providing a sink for a PipeWire stream). Where and how they are used should not be part of their name.

I personnly think ÙBDesktopPortal name is terrible 😄 because it's confusing and not other-devs-friendly. We must take care of the developer experience (DX), especially on an opensource project.

I agree.

If you ask 10 devs to tell what this class does actually in the code, how many will be able to tell without looking into it that it handles screen capture on Gnome and KDE ?

None. But nearly all of them can tell you that here something related to the XDG Desktop Portal is handled.

Completely personal perspective: Just how I would look on the code if I want to find the point where screenshots are taken: I could come from a very general point, not a specific function. When scrolling over the class names, I would find UBDisplayManager. In this class I would easily find grab(ScreenRole,...) and then dig down deeper from there.

If I am coming from e.g. the specific part of taking screenshots in Desktop mode, I would similar start from the UBDesktopManager and find a similar function, which is the using the UBDisplayManager. Note: this is the first step of the transition from a business class to a technical class. The display manager then invokes a function of UBPlatformUtils, which again uses either the Qt function or the desktop portal. And then we are really on the technical level.

However if I would find a class UBScreenShotDesktopPortal (see below), which prominently looks as if it would handle screen shots, then I would be tempted to set a breakpoint in this class to see what happens, and indeed find out that it is never called when I am in Board mode. And it would take me longer to dig upwards and find out, why and when we're using this function and when we're just grabbing a widget.

And I don't like either the "Linux" in the name, for the reasons mentionned above, but it must be clear in some way that it is linux specific. when we don't have the choice to have non-portable code.

Agreed.

And if we go for more interfaces to be handled in UBDesktopPortal, then its responsibility should be to manage the interfaces and that's all (SRP), and all the interfaces should have their own dedicated class (to respect ISP),

I also mostly agree here. In practice this creates many more classes, if you follow those principles to the end. And it can make code more difficult to follow, because it is more often crossing class boundaries and you have to have multiple editor windows open to get an overview.

In the following table : https://wiki.archlinux.org/title/XDG_Desktop_Portal, if we decide we want to interface with the Print portal tomorrow, then UBDesktopPortal class won't have to handle the print logic. We'll have a dedicated class, responsible for printing on linux using this xdg-desktop-portal interface, and ÙBDesktopPortal will be there maybe only to handle the DBus communication of the different interfaces, including this one, or just to instanciate them and manage them (just an example, probably not exactly what we should do)

If you check the documentation of the Print portal, you will see, that it mainly just sends a file to a printer, probably presenting a print dialog to the user. And yes, I would indeed add such a function to the UBDesktopPortal or UBDesktopPortalWrapper. When we're going to use more and more portals, then I would think about splitting it into UBScreencastDesktopPortal etc.

And actually, this would also be an option for me right now, splitting to UBScreencastDesktopPortal and UBScreenShotDesktopPortal.

So I can agree that UBLinuxScreenCastManager is not a good solution, if we look at the bigger picture, but still, UBDesktopPortal is not improving DX.

We can then find simple solutions that won't be perfect but at least help a bit, or go deep in the architecture and rework how we think this part of the code. Simple solution would be to rename it UBXDGDesktopPortalWrapper or moving it to a linux folder and rename it UBDesktopPortalWrapper. Option 3 would be to continue to discuss at the conception level, and see where it ends.

Option 3 is probably what will take the most time but provide the best benefits to OpenBoard, option 2 is OK for me, even option 1 could be OK.

I would currently opt for option 2, and also move UBPlatformUtils_linux to a respective subfolder and remove the _linux suffix there.

Also, it does not have to be solved today. I just took the chance of this discussion to put the subject on the table.

What do you think ?

My nickname is letsfindaway. So my advice: go in the right direction. You don't have to follow it to the end right now. The world is changing. If you strive for perfection, the ideal may have already shifted by the time you reach it.

[kaamui]

I think not each class has a "business" and there are classes, where a name reflecting their technical part is good. Look at the following examples:

• Classes reflecting entities, e.g. QFile, QScreen, UBDocument, ...
• Classes for technical tasks, e.g. QXmlReader, UBStringUtils, ...

You're right. It doesn't concern entities. For the technical tasks, I do think it's their business responsibility here, this is why I used the " " for the word business. I think the word itself is very badly representative of what it is supposed to mean here, but it's the one everybody uses so .. ^^

QXmlReader is responsible for reading XML, it's a perfect "business" name for me. It tells what it is responsible for.

I also mostly agree here. In practice this creates many more classes, if you follow those principles to the end. And it can make code more difficult to follow, because it is more often crossing class boundaries and you have to have multiple editor windows open to get an overview.

imho it creates way more benefits than it creates issues. I'm profundly convinced solid principles are key to long-term code structuration and development.

If you check the documentation of the Print portal, you will see, that it mainly just sends a file to a printer, probably presenting a print dialog to the user. And yes, I would indeed add such a function to the UBDesktopPortal or UBDesktopPortalWrapper. When we're going to use more and more portals, then I would think about splitting it into UBScreencastDesktopPortal etc.

But as you probably know I was going to answer, it would break OCP. If you have to modify the class when you extend the functionalities, then you probably missed you initial conception.

My nickname is letsfindaway. So my advice: go in the right direction. You don't have to follow it to the end right now. The world is changing. If you strive for perfection, the ideal may have already shifted by the time you reach it.

So you nickname should be letsfindtheway ;)

I agree. It might sound laughable, but I think naming things properly, is one fundamental part of the work. If you don't find easily the simple words to explain it, it's probably that something's wrong conceptually.

Like some french poet said long time ago : "Ce que l'on conçoit bien s’énonce clairement,
Et les mots pour le dire arrivent aisément" (Nicolas Boileau-Despréaux, 1674 <= he was probably also a C++ developer 👀)

translation : "What is clearly conceived is clearly expressed, and the words to say it come easily."

[letsfindaway]

I'm now playing around with moving and splitting the desktop portal classes, creating

• platform/linux/UBScreenshotDesktopPortalWrapper.h/.cpp and
• platform/linux/UBScreenCastDesktopPortalWrapper.h/.cpp

There is one function used by both: createRequestToken(). There is another similar function, however only used for screen cast: createSessionToken().

What would you do with those? There are several options:

• Maintain them in both classes and use another request token prefix to avoid clashes.
• Move them to a superclass.
• Move them to some DesktopPortalTokenGenerator class used by both.

I tend to the last solution.

[kaamui]

I like the idea of UBDesktopPortalTokenGenerator but having two functions doing quite the same thing, with no guarantee that it will not be misused bothers me.

We could imagine to have only one function in it, createToken and a map<string, int> (with an enum to not play with strings when using it). So when you need a token => UBDesktopPortalTokenGenerator::createToken(TokenKind::Session) (static or not I don't know, depends on who we imagine as responsible of its existence I guess)

[letsfindaway]

I like the idea of UBDesktopPortalTokenGenerator but having two functions doing quite the same thing, with no guarantee that it will not be misused bothers me.

We could imagine to have only one function in it, createToken and a map<string, int> (with an enum to not play with strings when using it). So when you need a token => UBDesktopPortalTokenGenerator::createToken(TokenKind::Session) (static or not I don't know, depends on who we imagine as responsible of its existence I guess)

Hmm, if I look at the requirements for tokens then I now even doubt that we have to differentiate between the two. They just have to be unique. There are several options now.

• We create a token generator with a single function, just counting the tokens to create unique ones.
• Instead of counting we use the random number as proposed.
• We do not use a generator at all, but use a UUID for the token.

Request and Session both state

The token that the caller provides should be unique and not guessable. To avoid clashes with calls made from unrelated libraries, it is a good idea to use a per-library prefix combined with a random number.

I have chosen different prefixes for session and request, and a counter instead of a random number, because OpenBoard is single-application and there are no problems with multiple instances. But actually there is no need to distinguish between session and request tokens.

Here is the specification of what a valid object path is. Length is not restricted, but

Each element must only contain the ASCII characters "[A-Z][a-z][0-9]_".

So if we would use a UUID, we would have to post-process the string. This is again an argument for using a specific method, which knows this restriction and creates a proper string.

I finally tend for such a single function using a random number, e.g.

class UBDesktopPortalTokenGenerator
{
public:
    static QString generateToken()
    {
        const auto randomValue = QRandomGenerator::global()->generate();
        return "OB_" + QString::number(randomValue, 16);
    }
}

What do you think?

[kaamui]

Perfect !