vionya / discord-rich-presence

A cross-platform Discord Rich Presence library written in Rust
MIT License
89 stars 16 forks source link

Useless lifetimes #19

Closed Bowarc closed 1 year ago

Bowarc commented 1 year ago

Hellow, im tring to have some sort of dynamic Activity, But the fact that activity::Activity uses Option<&'a str> instead of Option<String> + that it's returning self for each function makes that it's rly hard to use imo. I made a config-like structure to be used in .ron files so i can add or remove parts easily (I know it's supposed to be chained but i can't find a way to make it work with the optional config that i am using) Example:

let mut activity = activity::Activity::new();
match activity_config.state {
    Some(state) => {
        debug!("Activity::State set to: {}", state);
        activity = activity.state(state);
    }
    None => debug!("Activity::State not found"),
}
match activity_config.details {
    Some(details) => {
        debug!("Activity::Details set to: {}", details);
        activity = activity.details(details);
    }
    None => debug!("Activity::Details not found"),
}

This is impossible with the current type of activity::Activity The example above is working if you replace every Option<&'a str> by Option<String> and remove lifetimes on activity::Activity

It might just be me as im not very experienced with lifetimes, but i see no advantage of using Option<&'a str> instead of Option<String>.

antiaim commented 1 year ago

Agreed, was trying to run the ipc client in a thread but couldnt share the Activity struct with the thread because of the lifetimes. ended up having to just make another struct that contains the data and sharing that instead.

vionya commented 1 year ago

First thing to disclose: it's been a hot minute since I implemented the Activity interface, so I unfortunately cannot tell you my original thought process behind the lifetimes and everything.

That aside, would it be preferable to make a breaking change wherein instead of chaining, it instead has set_* operations that modify it in-place?

let mut activity = Activity::new();
match activity_config.state {
    Some(state) => {
        debug!("Activity::State set to: {}", state);
        activity.set_state(state);
    }
    None => debug!("Activity::State not found"),
}
match activity_config.details {
    Some(details) => {
        debug!("Activity::Details set to: {}", details);
        activity.set_details(details);
    }
    None => debug!("Activity::Details not found"),
}

Thanks for the thread :)

Bowarc commented 1 year ago

Yes, why not. Having functions like

Activity::set_state(&mut self, new_state: &str) -> () {
    self.state=new_state
}

Could be very helpfull for thoses kind of scenarios.

vionya commented 1 year ago

Alright, I'll plan to replace the current system with something like that in the next major version.

Bowarc commented 1 year ago

I tried a bit more and it turns out that, in my case, using the kw ref in the Some() section of the match compiles.

match activity_config.state {
    Some(ref state) => {
        debug!("Activity::State set to: {}", state);
        activity = activity.state(&state);
    }
    None => debug!("Activity::State not found"),
}
match activity_config.details {
    Some(ref details) => {
        debug!("Activity::Details set to: {}", details);
        activity = activity.details(&details);
    }
    None => debug!("Activity::Details not found"),
}

Even if that case worked; for other scenarios, having functions that take &mut self would be greatly appreciated.