scottlamb / moonfire-nvr

Moonfire NVR, a security camera network video recorder
Other
1.25k stars 138 forks source link

Add "Copy config" option to "Add camera" dialog #273

Closed sky1e closed 1 year ago

sky1e commented 1 year ago

When adding cameras in the config TUI, very often multiple cameras need very similar values in their configurations. This pull request adds a button to the "Add camera" dialog to fill in the fields from an existing camera.

image

This pull request is two commits, where the first is a bunch of tiny code improvements around the TUI I found while working, with no functional changes, and the second is the feature itself. I'm happy to implement any requested changes in how this feature could work. This just seemed to me like the nicest way to improve this use case. I don't personally use moonfire-nvr, but I'm working on it on request of friend who is.

scottlamb commented 1 year ago

Thanks for the PRs! I appreciate it, and I'm sure your friend does too.

When adding cameras in the config TUI, very often multiple cameras need very similar values in their configurations. This pull request adds a button to the "Add camera" dialog to fill in the fields from an existing camera.

Hmm, I'm debating the best spot for this button. I can imagine someone not noticing the copy button at the bottom until they've already gone to the effort of filling everything in. Or getting confused and overwriting tweaked values with it.

Another option might be right in the top-level edit cameras dialog via something like the following:

Edit cameras

<add camera from scratch>
<copy existing camera>
existing camera 1
...

<Done>

What do you think?

sky1e commented 1 year ago

Hmm, I'm debating the best spot for this button. I can imagine someone not noticing the copy button at the bottom until they've already gone to the effort of filling everything in. Or getting confused and overwriting tweaked values with it.

For where to put the copy config option, I was thinking that I didn't want to clutter the list of cameras too much with non-camera entries, although I suppose it makes sense to put it next to wherever the add new camera from scratch button is. I see what you mean by the possibility that the user doesn't notice the copy button until after it's too late to use it. I was thinking that copying an existing config and creating a new config from scratch to me both conceptually fall under different ways to add a new camera, and I'd imagine myself first thinking that I want to create a new config, and then deciding how to fill in the values, which aligns well with having one button to create a new camera and then deciding which to do on the create camera screen. Another possibility would be to put the copy existing config option at the top of the configuration screen, so one sees it before having filled in any fields.

scottlamb commented 1 year ago

Another possibility would be to put the copy existing config option at the top of the configuration screen, so one sees it before having filled in any fields.

I think that sounds great

sky1e commented 1 year ago

Just adding the button as the top element of the layout looks like this. What do you think? Should i left-align it?

image

scottlamb commented 1 year ago

Just adding the button as the top element of the layout looks like this. What do you think? Should i left-align it?

image

Yeah I think left alignment would make sense

scottlamb commented 1 year ago

Ergh, I was doing too many things at once, and hit the merge button before you actually made the change to position that we were discussing. Want to do a follow-up PR for that?