Skip to content
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

Center-view plugin #10

Merged
merged 1 commit into from
Jun 11, 2018
Merged

Conversation

AdrianVovk
Copy link
Contributor

@AdrianVovk AdrianVovk commented Jun 9, 2018

A plugin to open all normal windows in the center of the screen instead of the top-left corner.

Copy link
Member

@ammen99 ammen99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we said in IRC, this looks good, it's just minor style fixes so it looks like the rest of the code in the repo. Also in the end squash your commits together so it is a single commit.

signal_callback_t created_cb;

public:
void init(wayfire_config *config) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code style: brace on the next line

created_cb = [=] (signal_data *data)
{
auto view = get_signaled_view(data);
if (view->role == WF_VIEW_ROLE_TOPLEVEL && !view->parent) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code style: brace on the next line

};

extern "C" {
wayfire_plugin_t *newInstance() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code style: brace on the next line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the extern C? Or on the newInstance?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both

}
};

extern "C" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code style: brace on the next line

@@ -12,3 +12,4 @@ oswitch = shared_module('oswitch', 'oswitch.cpp', include_dire
rotator = shared_module('rotator', 'rotator.cpp', include_directories: [wayfire_api_inc, wayfire_conf_inc], dependencies: [wlroots, pixman, wfconfig], install: true, install_dir: 'lib/wayfire/')
apps_logger = shared_module('apps-logger', 'apps-logger.cpp', include_directories: [wayfire_api_inc, wayfire_conf_inc], dependencies: [wlroots, pixman, wfconfig], install: true, install_dir: 'lib/wayfire/')
window_rules = shared_module('window-rules', 'window-rules.cpp', include_directories: [wayfire_api_inc, wayfire_conf_inc], dependencies: [wlroots, pixman, wfconfig], install: true, install_dir: 'lib/wayfire/')
center_view = shared_module('center-view', 'center-view.cpp', include_directories: [wayfire_api_inc, wayfire_conf_inc], dependencies: [wlroots, pixman, wfconfig], install: true, install_dir: 'lib/wayfire/')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add another space so they line up :)
actually in the other columns as well

wf_geometry window = view->get_wm_geometry();
window.x = (workarea.width / 2) - (window.width / 2);
window.y = (workarea.height / 2) - (window.height / 2);
view->move (window.x, window.y);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the space between "move" and (

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops I've been using too much vala

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also some C++ codebases use it, it's a matter of preference (but code throughout a single codebase should generally follow the same style of course)

Copy link
Member

@ammen99 ammen99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things I missed before, sorry to bother you again

@@ -12,3 +12,4 @@ oswitch = shared_module('oswitch', 'oswitch.cpp', include_dire
rotator = shared_module('rotator', 'rotator.cpp', include_directories: [wayfire_api_inc, wayfire_conf_inc], dependencies: [wlroots, pixman, wfconfig], install: true, install_dir: 'lib/wayfire/')
apps_logger = shared_module('apps-logger', 'apps-logger.cpp', include_directories: [wayfire_api_inc, wayfire_conf_inc], dependencies: [wlroots, pixman, wfconfig], install: true, install_dir: 'lib/wayfire/')
window_rules = shared_module('window-rules', 'window-rules.cpp', include_directories: [wayfire_api_inc, wayfire_conf_inc], dependencies: [wlroots, pixman, wfconfig], install: true, install_dir: 'lib/wayfire/')
center_view = shared_module('center-view', 'center-view.cpp', include_directories: [wayfire_api_inc, wayfire_conf_inc], dependencies: [wlroots, pixman, wfconfig], install: true, install_dir: 'lib/wayfire/')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please align the = as well

@@ -0,0 +1,36 @@
#include <output.hpp>
#include <core.hpp>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need that include?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not, but what provides Wayfire_plugin_t?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plugin.hpp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ammen99 How is it working then? I'm not including that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some headers require others, e.g they aren't always self-contained

@@ -51,7 +51,7 @@ static wl_output_transform get_transform_from_string(std::string transform)
else if (transform == "180")
return WL_OUTPUT_TRANSFORM_180;
else if (transform == "270")
return WL_OUTPUT_TRANSFORM_180;
return WL_OUTPUT_TRANSFORM_270;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is that in this PR? maybe I forgot to push something

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idk. I think it merged in some commit at some point...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah if you rebase it should automatically drop this since it isn't part of this PR

@AdrianVovk
Copy link
Contributor Author

I've redone the commit again. Check it out again

@ammen99 ammen99 merged commit d61dc90 into WayfireWM:split-files Jun 11, 2018
@ammen99
Copy link
Member

ammen99 commented Jun 11, 2018

Thanks!

@AdrianVovk AdrianVovk deleted the split-files branch August 16, 2021 05:03
Kim-Dewelski pushed a commit to Kim-Dewelski/wayfire that referenced this pull request Dec 15, 2023
@killown killown mentioned this pull request Feb 16, 2024
This was referenced Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants