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

Add the ipc shutdown event #2335

Closed
wants to merge 1 commit into from

Conversation

acrisci
Copy link
Member

@acrisci acrisci commented May 2, 2016

This event is triggered when the connection to the ipc is about to
shutdown because of a user action such as with a restart or exit
command. The change field indicates why the ipc is shutting down. It
can be either "restart" or "exit".

fixes #2318

@stapelberg
Copy link
Member

This makes the tests fail. Can you add an appropriate skip statement that verifies AnyEvent::I3 is new enough? See e.g. t/220-ipc-window-title.t for an example.

@stapelberg stapelberg added the waiting Waiting for feedback/changes from the author of the issue/PR. Make sure to notify us with a comment. label May 6, 2016
@Airblader
Copy link
Member

@acrisci You've opened a bunch of initial pull requests which all fail Travis. Are you going to keep working on these?

@acrisci
Copy link
Member Author

acrisci commented Jun 21, 2016

Yeah sorry I've been kind of busy. Will try to finish this soon.

@acrisci
Copy link
Member Author

acrisci commented Jul 24, 2016

Ok, I finally fixed this up.

@acrisci acrisci removed the waiting Waiting for feedback/changes from the author of the issue/PR. Make sure to notify us with a comment. label Jul 24, 2016
@@ -9,9 +9,9 @@
* commands.c: all command functions (see commands_parser.c)
*
*/
#include <stdint.h>
Copy link
Member

Choose a reason for hiding this comment

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

Here and below, are the changes in include order and line breaks intentional? Does clang-format require them?

Copy link
Member

Choose a reason for hiding this comment

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

I think clang-format3.7 does such (ugly) line breaks. clang-format3.5, which we use, doesn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I just run the default clang-format that comes with my system and make all the changes it says to.

@stapelberg
Copy link
Member

Looks good overall.

This event is triggered when the connection to the ipc is about to
shutdown because of a user action such as with a `restart` or `exit`
command. The `change` field indicates why the ipc is shutting down. It
can be either "restart" or "exit".

fixes i3#2318
@acrisci
Copy link
Member Author

acrisci commented Aug 6, 2016

Ok I made the changes and it's ready for another review.

@@ -154,7 +154,8 @@ static Con *maybe_auto_back_and_forth_workspace(Con *workspace) {
*/
typedef struct owindow {
Con *con;
TAILQ_ENTRY(owindow) owindows;
TAILQ_ENTRY(owindow)
owindows;
Copy link
Member

Choose a reason for hiding this comment

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

This is still an incorrect line break

@Airblader
Copy link
Member

You didn't actually undo the incorrect formatting, did you? We really shouldn't use 3.7, the breaks are really ugly. We have an open issue to move to a current version.

@acrisci
Copy link
Member Author

acrisci commented Aug 6, 2016

OK wasn't paying that close attention to formatting. Is there documentation for this?

@Airblader
Copy link
Member

Is there documentation for this?

We use clang35 with the .clang-format file in the repository. I don't think there's further documentation.

We do have #2174, though I personally don't agree with @stapelberg's suggestion there anymore. I know code style is more about having a consistent style than it is about what that style is, but personally I think the formatting clang37 uses (out of the box, anyway) is really, really bad, and I'd not want to have it. I think we should try to configure newer versions of clang to keep the code formatted the way it is today.

@stapelberg stapelberg added the waiting Waiting for feedback/changes from the author of the issue/PR. Make sure to notify us with a comment. label Aug 13, 2016
@Airblader
Copy link
Member

@acrisci It's been two months (and longer for other PRs). Are you going to continue working on these PRs?

@Airblader
Copy link
Member

@acrisci I'd appreciate some feedback on this. My impression is that you no longer work on i3, but switched your focus to sway instead. In that case we can close the PRs.

@Airblader Airblader closed this Jan 13, 2017
@rsynnest
Copy link

rsynnest commented Oct 17, 2017

@Airblader From reading the comments I understand that this PR was valid but was in limbo due to a formatting issue with clang-format 3.5 -> 3.7.
Since you guys have moved to clang3.8 and accepted linebreaks (judging by #2547) , any chance you could reopen/merge this PR?
I ask because there are several bugs in an i3 plugin that should hopefully be fixed by this specific PR ( denesb/xfce4-i3-workspaces-plugin#30 altdesktop/i3ipc-glib#11 denesb/xfce4-i3-workspaces-plugin#25 denesb/xfce4-i3-workspaces-plugin#20 denesb/xfce4-i3-workspaces-plugin#7 ).

If you'd rather I submit a new PR I'd be happy to do so.

@Airblader
Copy link
Member

I don't have the link right now, but there was a new PR for the shutdown event which has been merged and is in 4.14. You can see on the IPC docs on the website that there is a shutdown event.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting Waiting for feedback/changes from the author of the issue/PR. Make sure to notify us with a comment.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement an ipc-shutdown event
4 participants