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

Use fmt for string formatting (GTK client) #3967

Merged
merged 1 commit into from Oct 15, 2022
Merged

Use fmt for string formatting (GTK client) #3967

merged 1 commit into from Oct 15, 2022

Conversation

mikedld
Copy link
Member

@mikedld mikedld commented Oct 15, 2022

No description provided.

@mikedld mikedld added scope:gtk type:refactor A code change that neither fixes a bug nor adds a feature labels Oct 15, 2022
@mikedld mikedld added this to the 4.0.0-beta.2 milestone Oct 15, 2022
@mikedld mikedld merged commit ae2dd5e into main Oct 15, 2022
@mikedld mikedld deleted the refactor/gtk-fmt branch October 15, 2022 16:13
@mikedld mikedld mentioned this pull request Oct 18, 2022
Copy link

@elfring elfring left a comment

Choose a reason for hiding this comment

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

Were changes mixed for different issues in this commit? 🤔

g_message(_("Got signal %d; trying to shut down cleanly. Do it again if it gets stuck."), sig);
gtr_actions_handler("quit", sighandler_cbdata);
}
g_message(_("Got termination signal, trying to shut down cleanly. Do it again if it gets stuck."));
Copy link

Choose a reason for hiding this comment

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

💭 Are you sure that the implementation of this function is async-signal-safe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I may have misread the question. The proper answer is: it doesn't matter now since g_warning() is no longer called inside the actual signal handler.

Copy link

Choose a reason for hiding this comment

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

I got the impression from your other response that this short feedback should be reconsidered another bit.
Are there different development views involved according to the term “signal handling” by the programming interfaces “POSIX” and “GLib”? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

POSIX signals and Glib signals are two completely different things which aren't related to each other. I'm sorry but I'd rather not dive into details here; I believe there's enough information on both topics out there if you're interested to know more.

@mikedld
Copy link
Member Author

mikedld commented Oct 19, 2022

Were changes mixed for different issues in this commit? 🤔

Changes to signal_handler() were initially necessary to get rid of printf-style formatting used in g_message() in favor of fmt. I then realised that using fmt in signal handler would be wrong and switched to g_unix_signal_add(), which allows to add handlers that are executed within Glib event loop, not as part of actual signal handling (answering your other question: yes, this is safe). This switch in turn required either passing signal number as part of user_data or via lambda capture, neither of which I wanted to do, so I decided to drop signal number printing altogether which resulted in not needing to use fmt, yielding the code you see in this PR.

@transmission transmission deleted a comment from ckerr Dec 1, 2022
@mikedld mikedld added the notes:none Should not be listed in release notes label Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes:none Should not be listed in release notes scope:gtk type:refactor A code change that neither fixes a bug nor adds a feature
Development

Successfully merging this pull request may close these issues.

None yet

2 participants