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 support for user-defined control codes #108

Merged
merged 3 commits into from
Nov 23, 2023
Merged

Conversation

vsrs
Copy link
Contributor

@vsrs vsrs commented Sep 21, 2023

Minor addition, no breaking changes.


This change is Reviewable

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vsrs)


src/service.rs line 82 at r1 (raw file):

        /// Can use user-defined control codes
        const NOTIFY = Services::SERVICE_USER_DEFINED_CONTROL;

Should we perhaps be consistent with the naming here? I.e., USER_DEFINED_CONTROL in this case.

Copy link
Contributor Author

@vsrs vsrs left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @dlon)


src/service.rs line 82 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should we stick to naming these the same as the Windows constants? I.e., USER_DEFINED_CONTROL in this case.

That makes sense, thank you. I've updated the code.

@vsrs
Copy link
Contributor Author

vsrs commented Nov 23, 2023

@dlon, may you please complete the review? Thanks.

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dlon dlon changed the base branch from main to notify November 23, 2023 11:52
@dlon dlon merged commit bed0710 into mullvad:notify Nov 23, 2023
10 checks passed
@dlon
Copy link
Member

dlon commented Nov 23, 2023

Thanks!

@vsrs vsrs deleted the notify branch June 16, 2024 14:56
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.

2 participants