Improve Save configuration UI - (re)allow for directly specifying custom path

[bd-g]

This addresses the discussion in #1659

PR Description:

Allows for either giving the user options of directories to save their files, but still allows them to type their own custom path if they'd like (now with tab completion). @svartkanin thoughts? There is some duplication here, but it does make it a little easier if you don't already have a path in mind.

Tests and Checks

• I have tested the code!

[svartkanin]

I'll have a look at this after the easter break :)

[svartkanin]

Okay I run through this a bit, some thoughts:

• My initial point still stands that I don't find the pre-computed directory list very necessary, but maybe that's just me :/ maybe @Torxed has a different opinion here?
• We're exclude a bunch of directories from the options, I understand that's to prevent overloading the size of options, but I still think that this actually removes these options completely from being selected, /opt, /var may be quite valid places to store the configs on the live ISO 🤷
• Could we remove the final confirmation question as I don't think it's really necessary and just puts another layer here, even if the user would have selected a wrong directory then the configuration may be saved in the wrong place, is that a big deal?
• On the pre-computed selection when pressing and keeping pressed the down arrow it iterates very slowly over the list, this doesn't seem to happen in other menus (e.g. the mirror list which is also quite large), are we computing things here in the background or is the list just way to big for proper rendering?
• Bug: Not entering any directory in the manual entry shouldn't go to the next question but just go back to the main menu as it's equivalent to Esc

[bd-g]

Thanks for the feedback!

I made this PR based on widely-used Usability Hueristics from the Nielson Norman group, so I'll respond to each point referencing my train of thought.

My initial point still stands that I don't find the pre-computed directory list very necessary, but maybe that's just me :/ maybe @Torxed has a different opinion here?

This was an attempt towards 6 Recognition Over Recall. The previous design was entirely recall based, and tab completion helps a little bit, but the pre-computed menu was an attempt to visually display your available options. Even if you could tab-complete to the same options, proactively displaying them for the user reduces the cognitive load of recalling the directory structure.

If both you and @Torxed still disagree, that's okay with me, and the most recent commit removes the pre-computed menu. However, if you change your mind I'd love to add it back!

We're exclude a bunch of directories from the options, I understand that's to prevent overloading the size of options, but I still think that this actually removes these options completely from being selected, /opt, /var may be quite valid places to store the configs on the live ISO shrug

If we choose to add the pre-computed menu back in, we can open back up /opt and /var. I don't have much experience with ISOs, which is why I asked in the original PR for feedback on the excluded directories. Are there any others you would want to be included that were being excluded?

Could we remove the final confirmation question as I don't think it's really necessary and just puts another layer here, even if the user would have selected a wrong directory then the configuration may be saved in the wrong place, is that a big deal?

The final confirmation aligns with a few usability concerns. First, 5 Error prevention, and 9 Help users recognize, diagnose, and recover from errors. The confirmation makes it easy to visually verify you chose the right directory.

Additionally, it addresses 1 Visibility of system status, which includes providing prompt and frequent feedback. The confirmation menu serves as sort of a "confirmation" that your file is being saved. Without it, you just press enter and it returns to the main menu, without necessarily confirming or denying where or if the file was saved. We could add a small temporary message indicating the file was saved, but that would have the same amount of friction as a confirmation menu, and then there would be an open question of how long to display the temp message.

Bug: Not entering any directory in the manual entry shouldn't go to the next question but just go back to the main menu as it's equivalent to Esc

Fixed

[bd-g]

I also think I'll have to tweak a few things once @svartkanin big PyParted PR gets merged, so I'll sit on that

[bd-g]

Having disk errors with new merged changes, so can't test the configuration save - will wait to adjust until I can get it up and testing.

[codefiles]

@bd-g are you getting the same disk error described here #1604 (comment)? If so would you mind testing the pull request #1754 and seeing if it resolves the issue and report back? I do not have the hardware needed to test and reproduce the the issue and would appreciate your assistance. Thanks!

[bd-g]

@bd-g are you getting the same disk error described here #1604 (comment)? If so would you mind testing the pull request #1754 and seeing if it resolves the issue and report back? I do not have the hardware needed to test and reproduce the the issue and would appreciate your assistance. Thanks!

That wasn't the exact error I was getting that I referenced above. However, I do have a different piece of hardware that has partitions in the /dev/nvmen* style. I'm busy today but I can try to replicate the error on that laptop tomorrow, and then if I can replicate it, test your PR.

[codefiles]

That would be awesome. If you provide more details on the error you are getting I will look into it. Having it documented with an issue would help if that is something you are willing to do when you have the time.

[bd-g]

That would be awesome. If you provide more details on the error you are getting I will look into it. Having it documented with an issue would help if that is something you are willing to do when you have the time.

Of course - I just haven't had the time to replicate the error several times and gather all the diagnostic info that is good for an issue, I was planning on opening an issue once I had the time to fully flesh it out

[svartkanin]

@bd-g I'm getting this error quite regularly as well

  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/home/dan/Git/archinstall/./archinstall/scripts/guided.py", line 254, in <module>
    ask_user_questions()
  File "/home/dan/Git/archinstall/./archinstall/scripts/guided.py", line 115, in ask_user_questions
    global_menu.run()
  File "/home/dan/Git/archinstall/archinstall/lib/menu/abstract_menu.py", line 377, in run
    if not self._process_selection(value):
  File "/home/dan/Git/archinstall/archinstall/lib/menu/abstract_menu.py", line 394, in _process_selection
    return self.exec_option(config_name, selector)
  File "/home/dan/Git/archinstall/archinstall/lib/menu/abstract_menu.py", line 415, in exec_option
    result = selector.func(presel_val)
  File "/home/dan/Git/archinstall/archinstall/lib/global_menu.py", line 174, in <lambda>
    lambda preset: save_config(self._data_store),
  File "/home/dan/Git/archinstall/archinstall/lib/user_interaction/save_conf.py", line 81, in save_config
    directories = SysCommand(file_picker_command).decode()
  File "/home/dan/Git/archinstall/archinstall/lib/general.py", line 451, in __init__
    self.create_session()
  File "/home/dan/Git/archinstall/archinstall/lib/general.py", line 502, in create_session
    with SysCommandWorker(
  File "/home/dan/Git/archinstall/archinstall/lib/general.py", line 282, in __exit__
    raise SysCallError(f"{self.cmd} exited with abnormal exit code [{self.exit_code}]: {self._trace_log[-500:]}", self.exit_code, worker=self)
archinstall.lib.exceptions.SysCallError: ['/usr/bin/find', '/', '-path', '/bin', '-prune', '-o', '-path', '/dev', '-prune', '-o', '-path', '/lib', '-prune', '-o', '-path', '/lib64', '-prune', '-o', '-path', '/lost+found', '-prune', '-o', '-path', '/opt', '-prune', '-o', '-path', '/proc', '-prune', '-o', '-path', '/run', '-prune', '-o', '-path', '/sbin', '-prune', '-o', '-path', '/srv', '-prune', '-o', '-path', '/sys', '-prune', '-o', '-path', '/usr', '-prune', '-o', '-path', '/var', '-prune', '-o', '-type', 'd', '-print0'] exited with abnormal exit code [1]: b"/systemd/user\x00/etc/systemd/user/sockets.target.wants\x00/etc/systemd/user/pipewire.service.wants\x00/etc/systemd/user/default.target.wants\x00/etc/opt\x00/etc/opt/chrome\x00/etc/opt/chrome/native-messaging-hosts\x00/etc/opt/edge\x00/etc/opt/edge/native-messaging-hosts\x00/etc/debuginfod\x00/etc/default\x00/etc/iproute2\x00/mnt\x00/mnt/rasperry\x00/mnt/rasperry/home\x00/mnt/archinstall\x00/mnt/archinstall/home\x00/mnt/archinstall/boot\x00/mnt/liveusb\x00/mnt/liveusb/data\x00/mnt/liveusb/efi\x00/mnt/backup\x00/usr/bin/find: '/mnt/backup': Input/output error\r\n"

which means if this fails, then the user won't get any options at all

[bd-g]

@bd-g I'm getting this error quite regularly as well

  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed

  File "/home/dan/Git/archinstall/./archinstall/scripts/guided.py", line 254, in <module>

    ask_user_questions()

  File "/home/dan/Git/archinstall/./archinstall/scripts/guided.py", line 115, in ask_user_questions

    global_menu.run()

  File "/home/dan/Git/archinstall/archinstall/lib/menu/abstract_menu.py", line 377, in run

    if not self._process_selection(value):

  File "/home/dan/Git/archinstall/archinstall/lib/menu/abstract_menu.py", line 394, in _process_selection

    return self.exec_option(config_name, selector)

  File "/home/dan/Git/archinstall/archinstall/lib/menu/abstract_menu.py", line 415, in exec_option

    result = selector.func(presel_val)

  File "/home/dan/Git/archinstall/archinstall/lib/global_menu.py", line 174, in <lambda>

    lambda preset: save_config(self._data_store),

  File "/home/dan/Git/archinstall/archinstall/lib/user_interaction/save_conf.py", line 81, in save_config

    directories = SysCommand(file_picker_command).decode()

  File "/home/dan/Git/archinstall/archinstall/lib/general.py", line 451, in __init__

    self.create_session()

  File "/home/dan/Git/archinstall/archinstall/lib/general.py", line 502, in create_session

    with SysCommandWorker(

  File "/home/dan/Git/archinstall/archinstall/lib/general.py", line 282, in __exit__

    raise SysCallError(f"{self.cmd} exited with abnormal exit code [{self.exit_code}]: {self._trace_log[-500:]}", self.exit_code, worker=self)

archinstall.lib.exceptions.SysCallError: ['/usr/bin/find', '/', '-path', '/bin', '-prune', '-o', '-path', '/dev', '-prune', '-o', '-path', '/lib', '-prune', '-o', '-path', '/lib64', '-prune', '-o', '-path', '/lost+found', '-prune', '-o', '-path', '/opt', '-prune', '-o', '-path', '/proc', '-prune', '-o', '-path', '/run', '-prune', '-o', '-path', '/sbin', '-prune', '-o', '-path', '/srv', '-prune', '-o', '-path', '/sys', '-prune', '-o', '-path', '/usr', '-prune', '-o', '-path', '/var', '-prune', '-o', '-type', 'd', '-print0'] exited with abnormal exit code [1]: b"/systemd/user\x00/etc/systemd/user/sockets.target.wants\x00/etc/systemd/user/pipewire.service.wants\x00/etc/systemd/user/default.target.wants\x00/etc/opt\x00/etc/opt/chrome\x00/etc/opt/chrome/native-messaging-hosts\x00/etc/opt/edge\x00/etc/opt/edge/native-messaging-hosts\x00/etc/debuginfod\x00/etc/default\x00/etc/iproute2\x00/mnt\x00/mnt/rasperry\x00/mnt/rasperry/home\x00/mnt/archinstall\x00/mnt/archinstall/home\x00/mnt/archinstall/boot\x00/mnt/liveusb\x00/mnt/liveusb/data\x00/mnt/liveusb/efi\x00/mnt/backup\x00/usr/bin/find: '/mnt/backup': Input/output error\r\n"

I'll look into it. I'm having to redo it too to fix some of the overwrites from the other PR. I'll try to get it done tomorrow?

I can also have it fail quietly potentially and just revert, instead of crashing the whole process.

[svartkanin]

@bd-g would you be able to update the PR so we can get this in?
If not, I can raise a minimal for the time being that just prevents the error from happening and make the configuration saving usable

[bd-g]

@bd-g would you be able to update the PR so we can get this in? If not, I can raise a minimal for the time being that just prevents the error from happening and make the configuration saving usable

Yea I can work on this.

[bd-g]

@svartkanin It looks like my previous code worked with just one small change to fix the type of an argument. I tested it through a few times and it seemed to work well - should be ready to merge in based on @Torxed opinion.

It allows for tab completion to any path, handles Esc and Ctrl+C at all points, and keeps the final confirmation menu to adhere to the usability standards I referenced earlier in this and other threads.

[svartkanin]

@Torxed shall we merge this as it resolves some annoying bug in the save configuration bug. I tested the PR and it works as expected

[Torxed]

I resolved a merge conflict, please take a look and see if I got the changes right :)

[bd-g]

I resolved a merge conflict, please take a look and see if I got the changes right :)

Seems fine - the upstream changes just deleted the file, so the merge looks like it just brought the changes back in. And yea, I had removed the SysCommand import too originally, but forgot to remove it the second time when upstreaming some other changes last week. Thanks, great catch!

[Torxed]

Sweet, there were some other minor tweaks like info() and debug().

Great work btw, and thanks for taking a second look after i poked around in the code ^^