Migrate i3-config-wizard to draw_util #2654

Merged
merged 1 commit into from Jan 25, 2017

Conversation

Projects
None yet
3 participants
@mihaicmn
Contributor

mihaicmn commented Jan 22, 2017

See #2642 for details.

- 0, /* X11 border = 0, we draw our own */
+ win, /* the window id */
+ root, /* parent == root */
+ WIN_POS_X, WIN_POS_Y, WIN_WIDTH, WIN_HEIGHT, /* dimensions */

This comment has been minimized.

@Airblader

Airblader Jan 22, 2017

Member

I don't like the hardcoded position, but it was like this before. We should fix this and while we're at it move mapping the window further down since we're currently violating the specifications. But that's for a separate PR, so this is just FYI.

@Airblader

Airblader Jan 22, 2017

Member

I don't like the hardcoded position, but it was like this before. We should fix this and while we're at it move mapping the window further down since we're currently violating the specifications. But that's for a separate PR, so this is just FYI.

+#define WIN_POS_X logical_px(490)
+#define WIN_POS_Y logical_px(297)
+#define WIN_WIDTH logical_px(300)
+#define WIN_HEIGHT (15 * font.height + TEXT_PADDING)

This comment has been minimized.

@Airblader

Airblader Jan 22, 2017

Member

This is now 1 * font.height more than it was before, no?

@Airblader

Airblader Jan 22, 2017

Member

This is now 1 * font.height more than it was before, no?

This comment has been minimized.

@mihaicmn

mihaicmn Jan 22, 2017

Contributor

No, previously it was a bit indirect: row_y(15) + font.height = 14 * font.height + logical_px(4) + font.height

@mihaicmn

mihaicmn Jan 22, 2017

Contributor

No, previously it was a bit indirect: row_y(15) + font.height = 14 * font.height + logical_px(4) + font.height

This comment has been minimized.

@Airblader

Airblader Jan 23, 2017

Member

Right. :-)

@Airblader

Airblader Jan 23, 2017

Member

Right. :-)

i3-config-wizard/main.c
@@ -463,82 +466,71 @@ void errorlog(char *fmt, ...) {
void debuglog(char *fmt, ...) {
}
+static void txt(int x, int row, char *text, color_t fg, color_t bg) {

This comment has been minimized.

@Airblader

Airblader Jan 22, 2017

Member

The calls to this use a lot of duplicated arguments for x. I think we should improve this as well. It's odd anyway to use pixels for x, but a row for y. We should switch the x argument to a column argument instead (and yes, this does mean using slightly different indentations). Do you want to do this in this PR or would you prefer it to be done separately?

@Airblader

Airblader Jan 22, 2017

Member

The calls to this use a lot of duplicated arguments for x. I think we should improve this as well. It's odd anyway to use pixels for x, but a row for y. We should switch the x argument to a column argument instead (and yes, this does mean using slightly different indentations). Do you want to do this in this PR or would you prefer it to be done separately?

This comment has been minimized.

@mihaicmn

mihaicmn Jan 22, 2017

Contributor

It makes sense. I will include the change in this PR.

@mihaicmn

mihaicmn Jan 22, 2017

Contributor

It makes sense. I will include the change in this PR.

@Airblader

This comment has been minimized.

Show comment
Hide comment
@Airblader

Airblader Jan 22, 2017

Member

Despite my comments, before merging I'm going to give this a shot on my machine and then let @stapelberg know.

Member

Airblader commented Jan 22, 2017

Despite my comments, before merging I'm going to give this a shot on my machine and then let @stapelberg know.

@mihaicmn

This comment has been minimized.

Show comment
Hide comment
@mihaicmn

mihaicmn Jan 22, 2017

Contributor

@Airblader I updated the code.

Contributor

mihaicmn commented Jan 22, 2017

@Airblader I updated the code.

@Airblader

This comment has been minimized.

Show comment
Hide comment
@Airblader

Airblader Jan 23, 2017

Member

I'm actually not sure when I can get around to setting up testing this on my machine. Do you mind posting a screenshot of what it looks like?

Member

Airblader commented Jan 23, 2017

I'm actually not sure when I can get around to setting up testing this on my machine. Do you mind posting a screenshot of what it looks like?

@mihaicmn

This comment has been minimized.

Show comment
Hide comment
@mihaicmn

mihaicmn Jan 23, 2017

Contributor

2017-01-23-225606_3840x2160_scrot
2017-01-23-225706_3840x2160_scrot

Contributor

mihaicmn commented Jan 23, 2017

2017-01-23-225606_3840x2160_scrot
2017-01-23-225706_3840x2160_scrot

@Airblader

Thanks!

@stapelberg stapelberg merged commit fa488d7 into i3:next Jan 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment