-
Notifications
You must be signed in to change notification settings - Fork 216
Move detection and calculation of grid snapping to grid plugin from move plugin #2916
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,18 +8,6 @@ namespace wf | |
| { | ||
| namespace grid | ||
| { | ||
| /** | ||
| * name: request | ||
| * on: core | ||
| * when: Emitted before move renders a grid indicator and sets the slot. | ||
| * carried_out: true if a plugin can handle move request to grid. | ||
| */ | ||
| struct grid_request_signal | ||
| { | ||
| /* True if a plugin handled this signal */ | ||
| bool carried_out = false; | ||
| }; | ||
|
|
||
| /** | ||
| * The slot where a view can be placed with grid. | ||
| * BL = bottom-left, TR = top-right, etc. | ||
|
|
@@ -38,6 +26,24 @@ enum slot_t | |
| SLOT_TR = 9, | ||
| }; | ||
|
|
||
| /** | ||
| * name: request | ||
| * on: core | ||
| * when: Emitted before move renders a grid indicator and sets the slot. | ||
| * carried_out: true if a plugin can handle move request to grid. | ||
| */ | ||
| struct grid_request_signal | ||
| { | ||
| /* True if a plugin handled this signal */ | ||
| bool carried_out = false; | ||
| bool force_off = false; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I needed some time to figure out what this does exactly, maybe we can rename it to
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| wf::output_t *output; | ||
| wf::point_t input; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one would need a docstring to clarify what it is actually (something like "input position relative to the indicated output")
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do. |
||
| int snap_threshold = 0; | ||
| int quarter_snap_threshold = 0; | ||
| wf::grid::slot_t slot_id = SLOT_NONE; | ||
| }; | ||
|
|
||
| /* | ||
| * 7 8 9 | ||
| * 4 5 6 | ||
|
|
@@ -118,5 +124,54 @@ inline wf::geometry_t get_slot_dimensions(wf::output_t *output, int n) | |
|
|
||
| return area; | ||
| } | ||
|
|
||
| /* Calculate the slot to which the view would be snapped if the input | ||
| * is released at output-local coordinates (x, y) */ | ||
| inline wf::grid::slot_t calc_slot(wf::output_t *output, wf::point_t point, int snap_threshold, int quarter_snap_threshold) | ||
| { | ||
| auto g = output->workarea->get_workarea(); | ||
|
|
||
| int threshold = snap_threshold; | ||
|
|
||
| bool is_left = point.x - g.x <= threshold; | ||
| bool is_right = g.x + g.width - point.x <= threshold; | ||
| bool is_top = point.y - g.y < threshold; | ||
| bool is_bottom = g.x + g.height - point.y < threshold; | ||
|
|
||
| bool is_far_left = point.x - g.x <= quarter_snap_threshold; | ||
| bool is_far_right = g.x + g.width - point.x <= quarter_snap_threshold; | ||
| bool is_far_top = point.y - g.y < quarter_snap_threshold; | ||
| bool is_far_bottom = g.x + g.height - point.y < quarter_snap_threshold; | ||
|
|
||
| wf::grid::slot_t slot = wf::grid::SLOT_NONE; | ||
| if ((is_left && is_far_top) || (is_far_left && is_top)) | ||
| { | ||
| slot = wf::grid::SLOT_TL; | ||
| } else if ((is_right && is_far_top) || (is_far_right && is_top)) | ||
| { | ||
| slot = wf::grid::SLOT_TR; | ||
| } else if ((is_right && is_far_bottom) || (is_far_right && is_bottom)) | ||
| { | ||
| slot = wf::grid::SLOT_BR; | ||
| } else if ((is_left && is_far_bottom) || (is_far_left && is_bottom)) | ||
| { | ||
| slot = wf::grid::SLOT_BL; | ||
| } else if (is_right) | ||
| { | ||
| slot = wf::grid::SLOT_RIGHT; | ||
| } else if (is_left) | ||
| { | ||
| slot = wf::grid::SLOT_LEFT; | ||
| } else if (is_top) | ||
| { | ||
| // Maximize when dragging to the top | ||
| slot = wf::grid::SLOT_CENTER; | ||
| } else if (is_bottom) | ||
| { | ||
| slot = wf::grid::SLOT_BOTTOM; | ||
| } | ||
|
|
||
| return slot; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_offeither - 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Certainly makes sense.