Move detection and calculation of grid snapping to grid plugin from move plugin #2916
Conversation
ammen99
left a comment
ammen99
left a comment
There was a problem hiding this comment.
overall I think this should work fine. I have a few nitpicks about the concrete implementation, as I would like to keep the signal as simple as possible.
| { | ||
| /* True if a plugin handled this signal */ | ||
| bool carried_out = false; | ||
| bool force_off = false; |
There was a problem hiding this comment.
I needed some time to figure out what this does exactly, maybe we can rename it to force_hide_preview or similar? Also I think that at this point we could rename the signal to grid_update_slot_preview_signal, it is a bit longer but it shows much more clearly what the signal is all about.
There was a problem hiding this comment.
I had trouble with naming that field and that definitely an improvement. I wanted to keep my changes to a minimum, so I didn't think about changing the signal name.
| bool carried_out = false; | ||
| bool force_off = false; | ||
| wf::output_t *output; | ||
| wf::point_t input; |
There was a problem hiding this comment.
This one would need a docstring to clarify what it is actually (something like "input position relative to the indicated output")
plugins/grid/grid.cpp
Outdated
| [=] (wf::grid::grid_request_signal *ev) | ||
| { | ||
| ev->carried_out = true; | ||
| wf::grid::slot_t new_slot_id = ev->force_off ? wf::grid::slot_t::SLOT_NONE : |
There was a problem hiding this comment.
Wouldn't it be way simpler if we let the move plugin calculate the slot (the helper function is available in the helper after all)? This way we don't have to pass the snap threshold, quarter snap threshold and we don't need force_off either - these can all live entirely in the move plugin.
Then in the signal we can maybe simply pass a slot_hint, which is a hint as to which slot should be activated. Then, your custom grid implementation (as you have said that you want to replace grid with custom slots) can recalculate the slot based on whatever thresholds and algorithms you want to.
There was a problem hiding this comment.
I disliked passing all of those variables as well, but I think it is necessary if keeping behavior identical. An alternate implementation of grid that is using regions different than the outputs otherwise wouldn't be able to use the same thresholds.
If the goal isn't to keep identical behavior, I would instead argue that the snap thresholds should live in the grid plugin and have a single snap threshold in the move plugin for when the grid plugin isn't in use. That can't be done because those same thresholds are used implicitly by the update_workspace_switch_timeout, which is now broken in the case where the grid plugin is not in use. I think it was already half-broken in that case, where the timer would keep getting reset if you keep moving the mouse even if you stay within the threshold. Both cases might not matter at all because that requires snapping to be enabled without a grid plugin loaded. I think the best solution would be to decouple workspace switching from snapping in the move plugin and doing a separate (though usually redundant) calculation for workspace switching and have it keep its own state.
There was a problem hiding this comment.
An alternate implementation of grid that is using regions different than the outputs otherwise wouldn't be able to use the same thresholds.
This is not true, these values come from the configuration file. Another grid implementation wishing to keep up with core's move plugin can query the same options from the config file. Any plugin can query any other plugin's options.
think the best solution would be to decouple workspace switching from snapping in the move plugin
Certainly makes sense.
BwackNinja
force-pushed
the
grid-handle-snap
branch
from
35c4736 to
724ed74
Compare
|
I've decoupled the workspace switching state from the snapping preview state and updated the naming. The snap thresholds are also pulled from the move plugin configuration by the grid plugin rather than passing them in the signal. Is that closer to what you're looking for? |
|
@BwackNinja seems quite good to me now, doesn't compile however (and I expect you'd have to fix the style, look at the CI output, there is a diff showing the needed changes) |
BwackNinja
force-pushed
the
grid-handle-snap
branch
from
724ed74 to
3dfc2a3
Compare
|
I compiled the wrong version when working between two computers. This should be better and takes care of the style issues. |
|
The clang build on Arch is known to fail currently due to a bug in a released version of meson. So that part is not your fault and requires no correction on your side. |
|
@BwackNinja For me, the move snap feature is completely broken now, does it work on your end? Have you tested it in a nested wayland-backend wayfire? |
|
Sorry. I messed that up in decoupling the state in the grid plugin from the state in the move plugin. Since the move plugin no longer knows the state when the grid plugin is handling the preview, The two options would be to either re-couple the slot_id state between the move and grid plugins, or to send a parameter in the |
Why not send a signal similar to the update slot preview signal to indicate that the view has been "dropped"? Maybe could even be an additional flag in the existing signal. Instead of just a bool indicating to stop the preview, it could be an enum with three states: start/update preview, finish drag'n'drop operation, or cancel the operation. |
|
This actually makes a lot of sense to me because move shouldn't be doing grid's slot calculation. Also, I think it would be nice to be able to separate outputs into subsections, given that there are monitors out there now that are wide enough to be used as if multiple outputs were side-by-side. Nice work! 👍 |
BwackNinja
force-pushed
the
grid-handle-snap
branch
from
3dfc2a3 to
2a29b9c
Compare
|
Okay. Now I've extended the signal with 3 operations, |
ammen99
left a comment
ammen99
left a comment
There was a problem hiding this comment.
LGTM now, seems to work, thanks!
|
If/when you implement your alternative grid plugin, feel free to share your work here or as a link in the GitHub wiki (https://github.com/WayfireWM/wayfire/wiki/Related-projects) :) |
|
Thanks! Initial implementation is here: https://github.com/BwackNinja/wayfire-plugin-grid-plus I need to stop using the grid's configuration options and use its own instead, but it does seem to work. |
3 participants
Second attempt working towards #1619
The grid plugin now responds to moves and shows previews unless it isn't loaded. When it isn't loaded, the move plugin still handles maximizing windows and the associated preview.