The brief period between a window being mapped and obtaining a surface
role did not respect pop-up reconfiguration. This adds a test for that,
along with a test that the toplevel's position cannot be reconfigured
after being mapped.
I triggered the unwrap in the `Motion` case while using the color picker
in Krita. If you hold down the mouse such that the color comparison
pop-up opens and flick to it, you can sometimes enter and motion on a
stale surface. Somewhat inconsistent to trigger.
I also removed the unwrap in the `Button` event since it is logically
equivalent to the other unwrap. A few logging statements also got
changed to give wl_surface details.
Since running the tests using all available threads is inconsistent, set
the default number of test threads to be 1. This can be overridden with
another use of the environment variable or with the `--test-threads`
flag. This improves packages which use `cargo test` as part of the build
process with no changes needeed from the packagers' side.
`RUST_TEST_THREADS` is documented as deprecated, but cannot be removed
by Cargo before a way to specify Cargo test flags becomes available.
`wl_clip_persist` hands satellite a `WritePipe` with the `O_NONBLOCK`
flag, which prevented more than 64 KiB (the Linux pipe capacity since
2.6.11) of selection data to be transferred. Since my research concluded
having `O_NONBLOCK` on Wayland's pipe transfer ends was probably valid,
I implemented basic handling for such a scenario.
I previously considered just using `fcntl` to remove the `O_NONBLOCK`
flag, but decided that was way too hacky, as it would not resolve the
underlying problem (which could easily be triggered by the receiving
program resetting `O_NONBLOCK` during the `write_all`.
The previous method of converting X selections had some flaws, including
one where XWayland would fail to send the necessary PropertyNotify
events if multiple INCR requests were sent to the same selection at
once. By using `write_to` to create a FIFO queue and `next_conversion`
to advance the queue, we can guarantee only one request is being
converted at a time, working around this issue.
Both PRIMARY and CLIPBOARD were using the same target atoms for their
property. When both are simultaneously requested (the behavior of
`wl-clip-persist --clipboard both`), the first would delete the property
the second was prepared to write data to, resulting in getting the
GetProperty reply containing the failure data.
This commit distinguishes `target` as the atom of a mime type contained
within `TARGETS`, and `property` as the atom which contains both data of
`target` and which selection requested it.
The SelectionClear event was previously calling `handle_new_owner`,
which relies on `SelectionClear` being sent only after a
`SetSelectionOwner` event. Even so, the
`XFixes::SelectionEvent::SetSelectionOwner` event already handles this
(and more obviously so). The result was that whenever Wayland owned the
X selection and X took it back, `handle_new_owner` would be called
twice, as would all of the machinery involved in acquiring a selection.
This behavior manifests as a bug if Wayland quickly reclaims ownership
of the selection, which can cause the selection event to be cancelled
before getting the SelectionNotify response is received, resulting in an
empty Wayland selection.
The clipboard and primary selections previously used the same atom to
store TARGETS when queried for them by `handle_target_list`. When a
program requests both selections simultaneously, problems such as one
selection using the target list of the other, then deleting it and
requiring the second target to get the selection targets again could
occur. This was observed when using `wl-clip-persist` and could result
in selection desyncs.
This change gives a unique atom to each primary and clipboard selection
targets so they no longer compete over the same property. It also adds
updates to the `copy_from_x11` intergration to test for this issue.
These simple decorations will be rendered only when the host compositor doesn't
support server side decorations and the X11 window does not render its own
decorations.
Closes#31
An unwrap was hit when a pointer event with no CurrentSurface was
processed. To resolve this, the same logic from the Event::Motion case
has been abstracted to a `handle_pending enter` lambda to provide a
valid CurrentSurface or return early if none could be entered.
Though the effect of `handle_pending_enter` remains, its flow has been
refactored, both to move the `None` case code outside the lambda and
to remove the unnecessary recursion. I would have used a `while let`
loop instead of the `loop let else` construct, but the borrow checker
did some mental gymnastics and rejected it.
The catch-all case of `handle_client_message` now logs the atom's name
instead of its opaque atom id.
`get_atom_name` now warns there was an error and returns a filler name.
Since most the use of `get_atom_name` is in logging, this makes those
logs easier to read.
The integration tests now use `formatted_timed_builder` logger
constructor, which should unify the look of logs between the integration
tests and the main binary.
LuckShiba's commit was not formatted with `rustfmt`, so I took the
opportunity to do some additional refactors.
Some bulky match constructions in xstate/selection were made more
compact as well.
Allows popups that would normally go offscreen to stay visible. X11 doesn't seem
to like windows whose bounds go less than 0x0, so we adjust the position
reported to X11 to make it happy.
I am worried this will have some other implications with popups, but we'll see
what happens.
This behavior was removed a while back, because we were also trying to activate
popups and this was causing issues on some compositors. However, I think
activating toplevels after they're mapped is the behavior that X11 apps tend to
expect - in Steam Big Picture for example, when launching a game in Niri while
fullscreened, the focus doesn't automatically switch to the game. This may need
to be reevaluated and refined in the future though.
Brought to my attention by #252, several `unwrap`s exist on code whose
failure does not justify the termination of the program, so I took some
time to replace them with proper error handling.
The request for TARGETS in `handle_selection_request` used `unwrap`.
Since the code for a failure of responsing to `SelectionRequest` exists,
it simply uses that.
`handle_target_list` now early returns if receiving the
`GetProperty` request fails.
`handle_new_owner` does not update the timestamp for the last selection
on failure.
`set_owner` now returns a `bool` to indicate whether the
`SetSelectionOwner` and `GetSelectionOwner` requests succeeded and
whether they transferred ownership successfully or not. The call spots
have also been modified to not change the `selection_state` if this call
fails.
Due to an oversight, if `XState` had both a valid primary selection
and a valid clipboard selection, `handle_selection_property_change`
would early return after `check_for_incr` determines the primary selection
existed but was not the recipient of the `PropertyNotifyEvent`, preventing
INCR events from ever reaching the clipboard. An addtion to the
`incr_copy_from_x11` integration test validates this fix.
Also fixed was a missing check in `WaylandSelection`'s `check_for_incr`
to confirm the property requesting more data is indeed the property the
selection is responsible for. I could not figure out how to write an
integration for this mistake, but it is obvious enough in hindsight.
Since the range end was always encoded in the data length, calculating
the end of the selection is a bit cleaner and removes a potentially
confusing slice operation on a slice.
Resolve Clippy's `too_many_arguments` lint on
`handle_selection_request` by removing the `success` and `refuse`
parameters and returning a bool indicating which needs to be run.
Use the helper functions defined last commit to simplify
`incr_copy_from_x11`
Reduce the `match` statement checking if the target is the TARGET
atom to an if statement. Although using `match` statements as
`if`/`else if`/`else` chains occur elsewhere, this one is both only 2
cases, and already of particular relevance to the current work.
Refactor `paste_impl` to be much easier to read. I did this code algebra
way back while investigating #142, but since I never figured that out,
the change has just been sitting in my stash.
refactor: simplify paste_data logic, some unnests
Since the connection handshake establishes the most data a request can
send, if the data length exceeds that limit, we can follow ICCCM 2.7.2
sending the INCR property and continuing to send data via PropertyNotify
events.
To test the changes, we create `XState::set_max_req_bytes` to forcefully
trigger the INCR mechanism in integration test runs with a constant,
substantially less amount of data.
I found an oversight where trying to transfer too much data (16 MiB
in my case) from a Wayland selection to an X window causes the X
connection to be unable to poll the event with a `ClosedReqLenExceed`
error.
To replicate, start `xwls`, use `wl-copy` to copy more data than the
`maximum-request-size` to the Wayland selection, then attempt to
transfer that selection to an X program. I found this easy to do
transferring a large, random `.bmp` file to Krita by creating its new
image from clipboard functionality.
This test replicates the observed behavior and obtains the same
panic, to be used as a starting point for implementing incremental
selections from Wayland to X.
xcb-util-cursor made another patch release to revert back to Rust 2021
edition, so we bump to that.
Also create a function, `timespec_from_millis` to make creating
`Timespec` for using in `poll` calls a lot more ergonomic.
By using conditional compilation, we now support running the test suite
with Rust versions 1.83 to 1.86 again. This allows us to lower the
`rust-version` specified in the root Cargo.toml (because it was
controlling the toolchain used in CI) to 1.83, resolving #230.
This solution keeps tests operational on our MSRV while also lowering
it. It would have been unsatisfying to have an MSRV which could not
compile the tests.
`rustversion` was selected as the dependency to control the conditional
compilation since it was already a build dependency needed by
`vergen-gitcl`, so no new dependencies were added.
