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

Refactor IPC handling in both client and daemon #333

Merged
merged 5 commits into from
Jun 26, 2024
Merged

Conversation

rkuklik
Copy link
Contributor

@rkuklik rkuklik commented Jun 25, 2024

This is "helper" pull request to consolidate IPC logic and simplify modification of IPC types.

@LGFae
Copy link
Owner

LGFae commented Jun 25, 2024

To maintain consistency, can you please move the daemon/src/daemon.rs file into daemon/src/daemon/mod.rs?

@rkuklik
Copy link
Contributor Author

rkuklik commented Jun 25, 2024

I can if you want (tomorrow), but don't you want to gradually move toward this style? Apart from being recommended, there already some modules using this style, in all three crates. Granted, they don't contain submodules (as far as I know), but to be consistent they would have to be also declared in mod.rs.

I would propose that after I finish working on IPC, we could unify style throughout the project, perhaps with lints like clippy::self_named_module_files (for exclusively mod.rs) or clippy::mod_module_files (doing opposite) and rustfmt options such as reorder_imports and imports_granularity for imports.

What do you think? And apart from imports, everything seems OK?

@rkuklik
Copy link
Contributor Author

rkuklik commented Jun 25, 2024

And speaking of changes, in the future, I would like to introduce fuzzing and miri to the mix, just to be sure with the piles of unsafe hiding in internals and untyped writing a reading to and from buffers. Don't know when I will get to that, but would you be fine if I introduce new testing crate (not included in releases) and add a few dependencies like libfuzzer to it?

@LGFae
Copy link
Owner

LGFae commented Jun 25, 2024

I can if you want (tomorrow), but don't you want to gradually move toward this style? Apart from being recommended, there already some modules using this style, in all three crates.

I see. I wasn't aware this is the recommended style. We can go with this then.

I would propose that after I finish working on IPC, we could unify style throughout the project

Yes, this would be pretty good. It's something I've never bothered doing, even though my coding style changed a lot since I began the project.

I would like to introduce fuzzing and miri to the mix

would you be fine if I introduce new testing crate (not included in releases) and add a few dependencies like libfuzzer

Yes, I don't mind more dependencies if they are just for testing or (dev, in general). However, miri can take a very long time to run the compression code tests, so we may need to have a look at that.


Incidentally, can I merge this PR, or are you planning on doing more stuff?

@rkuklik
Copy link
Contributor Author

rkuklik commented Jun 26, 2024

Merge please. But before release, how is it with testing? While I am pretty happy with produced code, there is no way there will not be some new subtle bug accidentally lurking about. I ran the test suite, but do you release betas or something like that? I wouldn't wan to just come in and break stuff.

@LGFae
Copy link
Owner

LGFae commented Jun 26, 2024

You can run cargo test -- --include-ignored for it to run some integrated tests. It will try out every command available, if I am not mistaken. Note you also need to shutdown the swww-daemon, if it's running on your machine.

Unfortunately, I couldn't find a way of automating testing that didn't involve "try everything and see if it works". Ultimately what I end up doing is just running the program on my setups (a desktop and a notebook with an extra monitor) and making sure it works ok on those. Fortunately, since we are only changing the IPC mechanisms, in principle, if the daemon and client can communicate, things should be fine. It is way more annoying when it's wayland stuff.


EDIT: also, before releasing, I need to transfer over some fixes/improvements I was working on before you did these major refactors, so it will be a little while still.

@LGFae LGFae merged commit 51f8637 into LGFae:main Jun 26, 2024
10 checks passed
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