Make feature flags for the "focused" and launcher "modules"

[thmasq]

Implementation to solve issue 403. Please tell me if there is something I should change or refactor.

Resolves #403.

[JakeStanger]

Thanks for the PR. As it is, the build is failing as you need to ensure code is formatted with cargo fmt .

There are also places that still need the feature flag (such as the mod statements in the Wayland client). If you compile with cargo build --no-default-features you will spot these, most likely as unused code warnings. Don't worry if you can't every single one of these, but try to put everything you reasonably can behind a flag.

Please also ensure you are using conventional commits. For example, build: add launcher feature flag. In this case since they are build commits they will not show in the changelog, but it helps keep the commit log standardised and easy to work with.

Let me know if you need a hand squashing/amending your existing commits.

Before this is merged, the docs will also need to be updated to include the new flags at a minimum. Let me know whether you're happy to take that on, and if not I can do that.

[thmasq]

I'm sorry for the trouble if that's the case. This is my first time contributing to someone else's repository, and I'm not quite experienced with git. This time I tested the changes with more combinations of the build flags to make extra sure it would compile, but there are still some warnings I haven't managed to fix. These warnings only show up when compiling without the focused and launcher features though. When compiling with default features I get no new errors (the only new warning I get is related to a match arm for on_request() events in src/clients/mod.rs. The compiler was requiring me to add this case, even though it is unreachable).

[thmasq]

Do you feel there's a need to add some note to the build documentation on what wayland protocols are needed to enable the focused and launcher modules? Perhaps the clipboard module as well. If not, I think this might be ready to merge.

[JakeStanger]

I'm sorry for the trouble if that's the case. This is my first time contributing to someone else's repository, and I'm not quite experienced with git.

No need to apologise. Git is a tricky beast, and this is fairly complex low level stuff. The only way to learn these things is to get in the weeds and try and find your way out, so I'm more than happy to lend a helping hand.

Do you feel there's a need to add some note to the build documentation on what wayland protocols are needed to enable the focused and launcher modules? Perhaps the clipboard module as well.

I think a little alert of the appropriate kind at the top of each applicable module page would be best. Just to say "this module requires a wlroots compositor" or something akin to that.

---

Thanks for updating - there's a few more bits I've noticed after reviewing if you can have a look at those please. I'll try and do a deeper dive this evening and compile for myself just to verify too.

Once that's all clear, we can have a look at squashing down the commits.

[thmasq]

I got the changes implemented now. I added notes in the docs as well, and for the sake of consistency, I put a note on the clipboard module to tell the user it requires the wlr-data-control protocol for to work. I don't think the other modules need notes as they only rely on standardized protocols

[JakeStanger]

I think that conflict has just been introduced by me merging in other PRs - let me know if you need my help resolving that. Otherwise, code lgtm now so thanks for that.

The note in the docs I think should clearly say you require a wlroots-based compositor like I had above. It's fine specifying the required protocol in there, but most people won't know much about the underlying protocols and will know if they're on a wlroots compositor.

Then lastly, if you can squash the commits down please. I think a single commit is best now, since both feature flags are kinda tied together:

build: add focused and launcher feature flags

Once that's done, I reckon we should be just about there.

[thmasq]

I think that's all changes implemented. Please review it, and tell me if there's annything that needs to be changed. Also I created another PR by accident, and I'm not sure if there's a way to delete it rather than just closing. I thought that using github's interface to sync the changes would be a good idea, but I was a fooled. Different from using the terminal, on the web interface it's hard to tell which changes are applied to remote before I can review them.

[JakeStanger]

Just having a look at the build results, and there are a lot of unused code warnings for code in various places when you run with --no-default-features . Some of them I think I'm happy to ignore as it's just a result of having next to no modules in the bar, but there are a couple still that I think would definitely make sense to put behind the feature flags:

warning: variant `ToplevelFocus` is never constructed
  --> src/clients/wayland/mod.rs:76:5

warning: variant `OpenPopupAt` is never constructed
  --> src/modules/mod.rs:76:5

If you can amend the commit to resolve those, and the related pattern matches, then I think that should be good enough.

[JakeStanger]

Also I created another PR by accident, and I'm not sure if there's a way to delete it rather than just closing.

I don't think there is, but don't worry about it. It's easy to muck it up, especially when you're using a GUI and you're not sure what it's doing under the hood.

[thmasq]

I tried putting those two variant constructors under either the focused and launcher flags with a cfg(any()), which eliminates the warning when I'm compiling with them disabled, but I still get the same warning when compiling with them. I'm not sure what to think of this

[JakeStanger]

I think you're making things more complex than they need to be.

I missed this when I reviewed because of the way GitHub shows only diffs (at least that's my excuse...) but when it comes to the match blocks, you can just stick the feature flag on the whole arm.

So for example, you currently have:

            Msg(Request::ToplevelFocus(id)) => {
                #[cfg(any(feature = "focused", feature = "launcher"))]
                let handle = env
                    .handles
                    .iter()
                    .find(|handle| handle.info().map_or(false, |info| info.id == id));

                #[cfg(not(any(feature = "focused", feature = "launcher")))]
                let handle: Option<()> = None;

                #[cfg(any(feature = "focused", feature = "launcher"))]
                if let Some(handle) = handle {
                    let seat = env.default_seat();
                    handle.focus(&seat);
                }

                send!(env.response_tx, Response::Ok);
            }

Instead, you only need (and want) one feature flag, so that the whole Msg(Request::ToplevelFocus(id)) arm never gets compiled in - this is required because you've stuck a flag on the actual ToplevelFocus definition in the enum. If the variant doesn't exist, you can't match against it.

(In this case too, ToplevelFocus is only used by launcher, so you don't need to compile it in for the focused module)

So this can become just:

            #[cfg(any(feature = "launcher"))]
            Msg(Request::ToplevelFocus(id)) => {
                let handle = env
                    .handles
                    .iter()
                    .find(|handle| handle.info().map_or(false, |info| info.id == id));

                let handle: Option<()> = None;

                if let Some(handle) = handle {
                    let seat = env.default_seat();
                    handle.focus(&seat);
                }

                send!(env.response_tx, Response::Ok);
            }

[thmasq]

Ok, got it. With this implemented there are considerably fewer warnings than before. Though there's one still pestering me quite a bit, which is this one:

warning: variant `ToplevelInfo` is never constructed
  --> src/clients/wayland/mod.rs:94:5
   |
86 | pub enum Response {
   |          -------- variant in this enum
...
94 |     ToplevelInfo(Option<ToplevelInfo>),
   |     ^^^^^^^^^^^^
   |
   = note: `Response` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis
   = note: `#[warn(dead_code)]` on by default

I tried removing that line of code (I didn't push it), and it compiled just fine for all combinations of feature flags, so I was wondering what it's for

[JakeStanger]

so I was wondering what it's for

Ah, that's a trick one just to confuse you. I added it during some refactoring last week, ended up not using it, then deleted it earlier today. You can ignore that one :)

All lgtm now, thanks for all your work on this!