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

#35: Add possibility to save recording right after stop clicked #41

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

datrax
Copy link

@datrax datrax commented Mar 2, 2021

No description provided.

@datrax
Copy link
Author

datrax commented Mar 2, 2021

I am also thinking of removing these checks

if (!recorderLogic.IsEnded || !recorderLogic.IsReplayable)
{
      MessageBox.Show("Nothing to save!");
      return;
}

...

   if (!recorderLogic.IsEnded || !recorderLogic.IsReplayable)
{
      MessageBox.Show("Nothing to export!");
      return;
 }

and just disabling the buttons properly on UI but it requires additional status (idle with recording in place) and a lot of extra work.

Ideally I would really like to simplify replay/pause/resume/stop buttons to just 2 ones. But I saw that you already answered that don't like this way. Anyway I may try to do that. As the interface might be overloaded soon. (I started to work on the dark theme).

@datrax datrax marked this pull request as ready for review March 2, 2021 21:44
@nguyenquyhy
Copy link
Owner

nguyenquyhy commented Mar 2, 2021

Actually I am doing the same change but a bit wider in scope so I think I'll close this PR for now.

I am also thinking of removing these checks

Yes agree, and this is included in my current work too. Basically I am trying to revamp the state system to resolve a bunch of related issues. I was a bit lazy doing this when I first released the plugin as I didn't know if anyone other than myself would use it 😂.

As the interface might be overloaded soon.

I'm not strongly against that, but I prefer to simplify when it is actually needed rather that beforehand. Sure when we add more features in we will have to change the UI here and there, but merging buttons just to add more buttons is not always the best way. Also, right now we still have plenty of space 😂.

Besides, what I am imagining for the future is that we will eventually have some buttons to switch "modes" (simplified, normal, complex), or we can do that responsively based on window size. But all those depends on what will actually be added.

@nguyenquyhy
Copy link
Owner

Just a tiny bit more context on why I don't prefer merging buttons without significant benefit: people tends to double click on buttons.
So if I want to really satisfy users, I have to consider the impact of each mis-click, and throw in some disabled timeout, and still that never feels correct to me (i.e. button changed to disabled for a little while, then change text). What I found is that have separate buttons for start/stop-like feature always give expected response from users and save me from worrying about unexpected impact.

@datrax
Copy link
Author

datrax commented Mar 3, 2021

Just a tiny bit more context on why I don't prefer merging buttons without significant benefit: people tends to double click on buttons.

Yeah, that makes sense. But the idea to have responsively based window size modes is also great.

OK then, I will continue with dark mode meanwhile.

Base automatically changed from master to main March 13, 2021 01:26
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