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

Enhacements to list_manager: #1346

Merged
merged 4 commits into from
Aug 1, 2022
Merged

Enhacements to list_manager: #1346

merged 4 commits into from
Aug 1, 2022

Conversation

wllacer
Copy link
Contributor

@wllacer wllacer commented Jun 22, 2022

Two small changes to ListManager:

  1. One more user modifiable method filter_options
    The idea is, given an object selected, filter (or if developer desires change the shown text) the available options, allowing to display only a subset.
    I have three potential use-cases for it:
    First (what i need) is, when having heterogeneous list, only the pertinent options for the type selected will be shown (imagine a list with disk/partitions or folder/files).
    The second use (just theoretical) is that the options shown can be filtered on user-defined security levels. Haven't yet found a use for us, but ...
    Third, to change the text of the option. f.i. in a user list, the promote/demote action could be shown just as demote or promote depends of which user is selected. Has its downsides -might need some code elsewhere- and might be not worth in this particular instance, but could be useful.

In this case I don't use a NotImplementedError exception but return the whole option list, as it is the default usage

  1. One public attribute last_choice
    It holds the last_choice selected by the user from the main list ONLY
    The main use-case is when we need to know if the list was exited either with a cancel or a confirm_and_exit and perform some different action after each instance. Usually cancel wouldn't do anything and confirm_and_exit change the state of parts of the system.

If such behaviour is desired coding should be a bit different from the usual ObjectList().run():

    my_list = MyList(...)
    result_list = my_list.run()
    if list.last_choice.value = my_list._confirm_action:
        process_list(result_list)
    else:
        pass

Other, more convoluted example is my actual need. I have a list of partitions to manage. I want to be able to delete partitions not only from the list but in the real hardware. This action has to be done after the list is fully processed, before anything else and NEVER if cancel is invoked.
First I create an attribute partitions_to_delete which will hold the list of real partitions to be deleted

class MyList(ListManager):
   def __init__(self,...):
      super().__init__(...)
      self.partitions_to_delete = []

This attribute is filled during normal processing (a partition marked to be deleted is removed from the list, and is space is freed to define new partitons to be created). I could resort to some code like before, but in this case i don't need code branching, but to ensure that the deletion list is empty unless a confirm action is requested, so i decided to overcharge the run() method.

	def run(self):
		result_list = super().run()
		if self.last_choice.value != self._confirm_action:
		    self.partitions_to_delete =  []
		return result_list, self.partitions_to_delete

@svartkanin have a look at it please

method filter_option. To filter options based on selected entry
attrib. last_choice. Which is the last action executed before exiting the loop
@wllacer wllacer requested a review from Torxed as a code owner June 22, 2022 22:32
@svartkanin
Copy link
Collaborator

svartkanin commented Jun 23, 2022

The main use-case is when we need to know if the list was exited either with a cancel or a confirm_and_exit and perform some different action after each instance

I think there's some viability to differentiating an exit of the menu to perform actions based on it so I think it might be handy

I want to be able to delete partitions not only from the list but in the real hardware

I haven't taken a look at the other PR yet but I think this sounds rather dangerous, are you saying that any action in the list menu will have possible consequences of actually running a command and creating/deleting things? If that is the case then than I think we should really not do that. Also having any bugs in this code might erase someones data without any intention what so ever so which is really bad.

@wllacer
Copy link
Contributor Author

wllacer commented Jun 23, 2022

I haven't taken a look at the other PR yet but I think this sounds rather dangerous, are you saying that any action in the list menu will have possible consequences of actually running a command and creating/deleting things? If that is the case then than I think we should really not do that. Also having any bugs in this code might erase someones data without any intention what so ever so which is really bad.

It is only an use case where i need buffered actions beyond the mere list management. And to control how it exits
I'm still not sure it will be a feature for general consumption. I did some tests, and while it was relatively secure with GPT disks; it turns a nightmare to get right with MBR ones (the latter might change the partition number live). But I think making the user needing parted/fdisk just to delete existing partitions might be a bit inconvenient.
I'm aware of the risks. Deleting anything is dangerous. But we do it with filesystems and whole disks without thinking, because we believe it is needed on our workflow (and hasn't been always error free) ...

@wllacer
Copy link
Contributor Author

wllacer commented Jun 27, 2022

@svartkanin You commented (doesn't appers to me at github but i got the mail)

In archinstall/lib/menu/list_manager.py:

@@ -44,6 +44,8 @@ def init(
self._base_actions = base_actions
self._sub_menu_actions = sub_menu_actions

  •  self.last_choice = None
    

Can we make this a private variable :)

Daniel , you know what I would answer, and I know what you would ;-) Assuming the theoretical (not pythonic) answer: public attributes only if they are to be referenced outside the class:. last_choice is explicitly defined for such usage (see the first sample code), so it ought to remain public
Most probably, we (defined as the set of hackers directly working on Archinstall code) will use the second form (overloading run), so it is indifferent. But, if a casual scripter ever reaches for the need of using ListManager, better leave an easy (but complete) interface available

@svartkanin
Copy link
Collaborator

My reasoning for initially suggesting a private variable (which I removed then again) was the use case for it, namely that it should be read-only basically.
An approach such as

self._test

@property
def test(self):
    return self._test

will prevent someone from trying to write the variable

some_class.test = 'something else'

because it throws an Exception.

I do understand that writing code like that gets annoying to implement since it is way more verbose and seems unnecessary for such a small thing but in general it does help to prevent bugs from happening

So yeah I do agree that here it's fine to keep it that way as is :)

@wllacer
Copy link
Contributor Author

wllacer commented Jun 28, 2022

You're right with the read-only character. I'll update the code accordingly.

@wllacer
Copy link
Contributor Author

wllacer commented Jul 19, 2022

I've added last_choice to selection_menu also

@wllacer wllacer mentioned this pull request Jul 21, 2022
10 tasks
@Torxed Torxed merged commit 3bc3922 into archlinux:master Aug 1, 2022
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.

None yet

3 participants