Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Input Capture Desktop Portal #268

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

3l0w
Copy link

@3l0w 3l0w commented Sep 26, 2024

Describe your PR, what does it fix/add?

This PR implements the input capture desktop portal with the new protocol defined in hyprwm/hyprland-protocols#8.

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

Nothing to declare yet.

Is it ready for merging, or does it need work?

It work when used with hyprwm/Hyprland#7919, but still need work, like sending the keyboard layout, or security concerns.

@@ -1,6 +1,7 @@
[submodule "hyprland-protocols"]
path = subprojects/hyprland-protocols
url = https://github.com/hyprwm/hyprland-protocols
url = https://github.com/3l0w/hyprland-protocols
branch = feat/input-capture-impl
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

marking this one as a blocker until this is removed so we dont forget (don't resolve)

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit of a lengthy review, mostly style and guidelines. Many comments apply to the entirety of the PR

transform = (wl_output_transform)transform_;
output->setMode([this](CCWlOutput* r, uint32_t flags, int32_t width, int32_t height, int32_t refresh) {
refreshRate = refresh;
this->width = width;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no this-> here and everywhere else

for conflicting local and member names, use local_ e.g. int32_t width_ and then width = width_

@@ -32,6 +33,9 @@ struct SOutput {
uint32_t id = 0;
float refreshRate = 60.0;
wl_output_transform transform = WL_OUTPUT_TRANSFORM_NORMAL;
uint32_t width, height;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ALWAYS initialize member primitives

CInputCapturePortal::CInputCapturePortal(SP<CCHyprlandInputCaptureManagerV1> mgr) {
Debug::log(LOG, "[input-capture] initializing input capture portal");
m_sState.manager = mgr;
sessionCounter = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be moved to the header


CInputCapturePortal::CInputCapturePortal(SP<CCHyprlandInputCaptureManagerV1> mgr) {
Debug::log(LOG, "[input-capture] initializing input capture portal");
m_sState.manager = mgr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CInputCapturePortal::CInputCapturePortal(SP<CCHyprlandInputCaptureManagerV1> mgr) : m_sState.manager(mgr) {

m_pObject->registerMethod(INTERFACE_NAME, "Release", "osa{sv}", "ua{sv}", [&](sdbus::MethodCall c) { onRelease(c); });
m_pObject->registerMethod(INTERFACE_NAME, "ConnectToEIS", "osa{sv}", "h", [&](sdbus::MethodCall c) { onConnectToEIS(c); });

m_pObject->registerProperty(INTERFACE_NAME, "SupportedCapabilities", "u", [](sdbus::PropertyGetReply& reply) { reply << (uint)(1 | 2); });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint is quite ambiguous, use uint32_t

return 0;
}

this->client.handle = client;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no this->

src/shared/Eis.cpp Show resolved Hide resolved
}

void EmulatedInputServer::ensurePointer(eis_event* event) {
if (client.pointer != nullptr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!= nullptr redundant

if (client.pointer != nullptr)
return;

struct eis_device* pointer = eis_seat_new_device(client.seat);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C-style structs, drop the struct

#include <string>

struct EisClient {
struct eis_client* handle;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no C-style structs + initialize

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants