-
Notifications
You must be signed in to change notification settings - Fork 169
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
Add place plugin #18
Add place plugin #18
Conversation
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.
Minor nitpicks, overall this is good
plugins/single_plugins/place.cpp
Outdated
#include <workspace-manager.hpp> | ||
#include <signal-definitions.hpp> | ||
|
||
using namespace std; |
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.
don't using namespace std;
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.
Ok.
plugins/single_plugins/place.cpp
Outdated
auto view = get_signaled_view(data); | ||
|
||
if (view->role != WF_VIEW_ROLE_TOPLEVEL || | ||
view->parent ||view->fullscreen || |
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.
space after the first ||
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.
Oops.
plugins/single_plugins/place.cpp
Outdated
|
||
auto workarea = output->workspace->get_workarea(); | ||
auto section = config->get_section("place"); | ||
auto placement_mode = section->get_option("mode", "center"); |
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.
just a minor thing, you get get the section + option in init()
, and save the placement_mode as wf_option
, but it's fine now as well
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 don't know what you mean by this.
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.
Just that you can store the result of get_option() as a member variable, but this isn't a big problem
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 see now.
plugins/single_plugins/place.cpp
Outdated
|
||
view->move(cascade_x, cascade_y); | ||
|
||
cascade_x += 50; |
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.
are we sure this should be done this way (hardcoded values)? Maybe add 2-3% of the resolution?
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.
It could be done any way, I think the main idea for this primitive placement mode is so windows are not placed directly on top of each other, so the user can see them as they open.
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.
but, on a 4k monitor and a low res one the plugin would have a different(IMO) behavior. How does compiz do it or you don't know?
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.
Yes I see what you mean. https://github.com/compiz-reloaded/compiz/blob/master/plugins/place.c#L755 is where compiz increments cascade position. input refers to the decoration, so it's using the decoration size which is basically a constant in this case, so we likely should choose something else that better suits this platform. 3% sounds like a good start to me.
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 think decoration adapts to the screen density/resolution so probably we'd need a percentage. choose whatever seems good to you
Add a basic window placement plugin with three placement modes. This plugin was based off of the center-view plugin by Adrian Vovk. It contains the same code as center-view for the center placement mode. The valid mode config keys are cascade, center and random.
LGTM, will merge it once I am able to get back to my computer and test it |
Works as a charm, thanks! |
Add a basic window placement plugin with three placement modes.
This plugin was based off of the center-view plugin by Adrian
Vovk. It contains the same code as center-view for the center
placement mode. The valid mode config keys are cascade, center
and random.