canonical / mir-ci

Mir CI helpers
1 stars 1 forks source link

Track pointer position and add Release Buttons keyword #99

Closed hbatagelo closed 3 months ago

hbatagelo commented 4 months ago

Fixes #97 and #98.

New keywords:

Remark: when WaylandHid is created, the pointer is moved to (0,0) and the tracking starts from that position.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 63.88%. Comparing base (c7ae89a) to head (bedc74d). Report is 7 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #99 +/- ## ======================================= Coverage 63.88% 63.88% ======================================= Files 11 11 Lines 742 742 Branches 103 103 ======================================= Hits 474 474 Misses 243 243 Partials 25 25 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Saviq commented 4 months ago

I'd say we should drop the From… variants, they just muddy the waters and may cause unexpected things.

I mean:

Move Pointer To ${template}
Walk Pointer To ${template_two}

vs.

Walk Pointer From ${template} To ${template_two}

There's an implicit Move at the beginning of the Walk, and there's not really that much less to type?

Saviq commented 4 months ago

(toggled draft so it picks up the changes in main)

hbatagelo commented 4 months ago

I agree. I'll try to push Walk into the library, remove the clutter and hide the implementation details... and use tuples of ints instead of dictionaries.

hbatagelo commented 4 months ago

Note to myself:

Saviq commented 4 months ago

I'll try to push Walk into the library

Realized there's at least one reason why some of those keywords actually fit into a resource… and that's combining functionality from multiple libraries (like Move To ${template}, Walk To ${template}). Don't think we necessarily want WaylandHid to hard-depend on Screencopy, but rather on its interface. Say we want to introduce a VNC client to read the frames.

I suppose it could be done through library arguments somehow, but not sure it's worth it. And any case, not this PR.

hbatagelo commented 4 months ago

Oh, indeed, I agree that keywords that use multiple libraries should stay in the resource file. I'll keep that in mind.

hbatagelo commented 4 months ago

Latest changes (addresses code review feedback and fixes #102):

Now, in WaylanHid, we have:

In the resource file:

These last two had to stay in the resource file (as opposed to Move Pointer To (${x}, ${y}) and Move Pointer To Proportional (${x}, ${y})) because of the optional arguments. The optional arguments are positional arguments, but unfortunately Robot doesn't allow mixing positional args with embedded args in the library.


Here's a small script for showing the different keyword syntaxes:

Move Pointer To (200, 100)
Move Pointer To ${SRC_TEMPLATE}
Move Pointer To Proportional (1, 1)
Walk Pointer To (200, 100)
Walk Pointer To ${SRC_TEMPLATE}
Walk Pointer To Proportional (1, 1)

${p1}=    Walk Pointer To Proportional (0.5, 0.5)
${p2}=    Displace ${p1} By (200, 50)
Walk Pointer To ${p2}            # Same as Walk Pointer To (${p2}[0], ${p2}[1])
Move Pointer To ${p1}            # Same as Move Pointer To (${p1}[0], ${p1}[1])

Some thoughts/remarks:

hbatagelo commented 3 months ago

We need ROBOT_AUTO_KEYWORDS=False:

https://robotframework.org/robotframework/latest/RobotFrameworkUserGuide.html#creating-keywords

It's a shame that we have to repeat keywords in the resource just for the sake of documenting them, WDYT?

On the other hand, it kind-of serves as an interface?…

We use the @library decorator which implicitly sets ROBOT_AUTO_KEYWORDS to False.

Well, I like the resource acting as a high-level interface so that WaylandHid doesn't need to depend on Screencopy.