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

Fix create and queue not working with 1 param in IPC mode #185

Merged
merged 4 commits into from
Apr 30, 2023
Merged

Fix create and queue not working with 1 param in IPC mode #185

merged 4 commits into from
Apr 30, 2023

Conversation

TheRustyPickle
Copy link
Contributor

@TheRustyPickle TheRustyPickle commented Apr 27, 2023

Previously on IPC mode when parsing break and work time, it would return an error if either was absent. Now modified to use the default value for the missing parameter. Resolves #184

@24seconds 24seconds added the bug Something isn't working label Apr 27, 2023
@24seconds
Copy link
Owner

Hello, @TheRustyPickle ! How's it going? I saw you are working on rex 👍👍👍. Thank you for the contribution again!

Aha.. at that time, to not to make a discrepancy between ipc mode and standalone mode, I tried to use shared function. parse_work_and_break_time is the one of my intention. But currently it's not shared.
So.. How about modify this function so that it can be shared in here

let mut work_time = match configuration.get_work_time() {
Some(work_time) => work_time,
None => DEFAULT_WORK_TIME,
};
let mut break_time = match configuration.get_break_time() {
Some(break_time) => break_time,
None => DEFAULT_BREAK_TIME,
};
if matches.get_one::<String>("work").is_some() {
work_time =
parse_arg::<u16>(matches, "work").map_err(NotificationError::NewNotification)?;
}
if matches.get_one::<String>("break").is_some() {
break_time =
parse_arg::<u16>(matches, "break").map_err(NotificationError::NewNotification)?;
}
?

@TheRustyPickle
Copy link
Contributor Author

Hello, @24seconds! I'm fine, thank you. Hope you are doing good as well. Will be working on this soon. Thanks for the heads up!

@24seconds 24seconds self-requested a review April 28, 2023 20:53
@24seconds
Copy link
Owner

Thank you for the quick action @TheRustyPickle .
I think we need to think about configuration.

When you think about ipc mode, there are two components. The rust-cli-pomodoro app which is acting as ipc server and the rust-cli-pomodoro app which is acting as ipc client.
Configuration is usually used when the rust-cli-pomodoro app starts up as standalone mode or ipc server.

async fn detect_command_type() -> Result<CommandType, ConfigurationError> {
let matches = command::get_start_and_uds_client_command().get_matches();
debug!("handle_uds_client_command, matches: {:?}", &matches);
let command_type = match matches.subcommand().is_none() {
true => CommandType::StartUp(get_configuration(&matches)?),
false => {
if let Some(val) = matches.subcommand_matches("completion") {
CommandType::AutoComplete(val.to_owned())
} else {
CommandType::UdsClient(matches)
}
}
};
Ok(command_type)
}

Therefore, I think considering configuration in ipc client is not proper.

[Suggestion]
The proper behavior of q -w 12 in ipc mode would look like below.

  • rust-cli-pomodoro app is running with some configuration. Let say break_time_default_value is 10.
  • Typed pomodoro q -w 12 in another terminal tab. There is no configuration related command here.
  • Running rust-cli-pomodoro app should handle this without problem and creates a notification with 12 minutes work and 10 minutes break. Note that configuration is used in running rust-cli-pomodoro.

To do this, maybe parse_work_and_break_time function should take Option of Arc<Configuration? (I don't know exactly. Fill free to modify the function so that rust-cli-pomodoro works properly as I described)

What do you think about this? Could you take a look?

@TheRustyPickle
Copy link
Contributor Author

Thanks for your input, @24seconds. Please correct me if I misunderstood, the configuration should only be used in the standalone mode. In IPC mode, it should ignore any configuration and use DEFAULT_WORK_TIME and DEFAULT_BREAK_TIME accordingly for missing the parameter if any.

@24seconds
Copy link
Owner

Thanks for your input, @24seconds. Please correct me if I misunderstood, the configuration should only be used in the standalone mode. In IPC mode, it should ignore any configuration and use DEFAULT_WORK_TIME and DEFAULT_BREAK_TIME accordingly for missing the parameter if any.

Ah, this is quite tricky part. I should revisit this later.
Actually ipc server mode is standalone mode. The first rust-cli-pomodoro app started works as standalone mode. And it can serve ipc client request. If you start rust-cli-pomodoro app in another terminal tab, then that would just work as standalone mode.

Let say we started pomodoro 1, pomodoro 2 and pomodoro 3 as standalone mode. Then the early created pomodoro 1 app would work as ipc server mode.
When you typed pomodoro q -w 12 in another terminal tab, then pomodoro 1 is in charge of handling the request.
Therefore, pomodoro 1's configuration should be considered when handling the q -w 12 ipc request.

Run the app and try some commands, you will understand @TheRustyPickle

@TheRustyPickle
Copy link
Contributor Author

Thanks for the explanation. I tried the app already, I misunderstood some of the terms 🙏. I got what you mean now. Will work on fixing this.

@24seconds
Copy link
Owner

I guess maybe you are not the only one who misunderstands the term. Let me follow up clarifying the terms. 🙇

@24seconds 24seconds mentioned this pull request Apr 28, 2023
@TheRustyPickle
Copy link
Contributor Author

Hello again, @24seconds, the latest commit should fix the issues we talked about earlier. Do take a look when you are available and let me know what you think🙏

Copy link
Owner

@24seconds 24seconds left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the contribution!

@24seconds 24seconds merged commit 92a5f8e into 24seconds:main Apr 30, 2023
3 checks passed
@TheRustyPickle TheRustyPickle deleted the ipc-queue-fix branch April 30, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] ipc mode q -w 12 not work.
2 participants