dw-0 / kiauh

Klipper Installation And Update Helper
GNU General Public License v3.0
3.4k stars 492 forks source link

feat: add Obico for Klipper to KIAUH #227

Closed kennethjiang closed 2 years ago

kennethjiang commented 2 years ago

Hi @th33xitus, I think this PR is ready for review now. Sorry for the delay.

Here are a few areas that I will need your feedback on:

  1. I have been naming the systemd service after the pattern of moonraker-obico[-xxx].service, which caused some problem with existing code. Since moonraker-obico has gone live for a while, I don't want to break the compatibility with the users already using it, I had to make changes here and here. The 2nd one is quite ugly. Let me know if you have a better idea.
  2. moonraker-obico can be in an installed but *not linked" status. I made the change here and here. Again this is a bit of inverted dependency because now main_menu.sh and install_menu.sh depend on functions in obico.sh. Will love to hear your feedback here too.

Screenshots for single-instance installation:

Screen Shot 2022-08-01 at 3 17 22 PM Screen Shot 2022-08-01 at 3 17 46 PM Screen Shot 2022-08-01 at 3 17 32 PM Screen Shot 2022-08-01 at 3 18 03 PM

Screenshots for multi-instance installation:

Screen Shot 2022-08-01 at 6 21 59 PM Screen Shot 2022-08-01 at 6 24 55 PM

dw-0 commented 2 years ago

Hi @kennethjiang, on first glance it looks like its going into the right direction. I saw one or two oddities, but as you stated that this is a rough draft, i'll wait until you give the go for me to start the formal review.

For example:

kennethjiang commented 2 years ago

@th33xitus I changed this PR based on our discussion and tested all the cases I can think of.

I found 1 bug: on the install_menu screen, if the user selects 1) [klipper], 2) [moonraker], and 9) [Obico for Klipper] without going back to the main screen, function get_multi_instance_names will return an empty string.

I did some debugging and found that .kiauh.ini is not updated until the user goes back to the main screen. However, I struggled to figure out where I did wrong. Will appreciate it if you can provide some guidance here.

Please also go over the code again, particularly commits 7837a78c4b6113e76dc883058118c0a21eef3410, 7837a78c4b6113e76dc883058118c0a21eef3410, and 8d284179d8a21f7ef6720d5c9388a27e7fce58c5

dw-0 commented 2 years ago

I found 1 bug: on the install_menu screen, if the user selects 1) [klipper], 2) [moonraker], and 9) [Obico for Klipper] without going back to the main screen, function get_multi_instance_names will return an empty string.

I did some debugging and found that .kiauh.ini is not updated until the user goes back to the main screen. However, I struggled to figure out where I did wrong. Will appreciate it if you can provide some guidance here.

You didn't do any wrong. You actually pointed me to a maybe not so smart solution. https://github.com/th33xitus/kiauh/blob/master/scripts/ui/main_menu.sh#L85

The init_ini function is called whenever the main menu is loaded. https://github.com/th33xitus/kiauh/blob/master/scripts/utilities.sh#L125

And in there the multi instance names are then written to the ini https://github.com/th33xitus/kiauh/blob/master/scripts/utilities.sh#L189

Maybe it is smarter to factor fetch_webui_ports and fetch_multi_instance_names out of the init_ini function and place them both into the install_menu function right after install_ui. Both those fetch functions aquire data that is usually only used in installation functions, at least i can't think of any other usecases within the script right now.

dw-0 commented 2 years ago

@kennethjiang so i just pushed a commit addressing the bug you mentioned.

I have a view comments to make on other pieces of the code, i quite don't understand yet.