search

spyder-ide / ux-improvements

Discussion about UX improvements for Spyder 5 and beyond
4 stars 2 forks source link

Add menu for list/tables (from double click) to a static toolbar #67

Closed dpturibio closed 2 years ago

dpturibio commented 2 years ago

Issue Report Checklist

Problem Description

Enhancement: I would like to propose a enhancement for the menu popped when a list/table is right-clicked. As my team is frequently working on lists and tables in our internal Spyder Plugin, we noticed that right-clicking every time in a row to show table/list's options is not the best way to do this. With this in mind, I would like to suggest incorporating a static toolbar in the top of the list/table window as shown in the picture bellow.

Screenshot_spyder

What steps reproduce the problem?

  1. Create any list in the editor, for example: test_list = [1,2,3,4,5]
  2. Click in Run->Run with "IPython console" and "Variable explorer" active
  3. Double-click in test_list variable in "Variable explorer" window.
  4. In the opened window, right click on any item and menu will show up

What is the expected output? What do you see instead?

Expected output is to have a static edition toolbar in the window further on the right-clicked menu If this enhancement is approved, I can work on it as previously worked on spyder-ide/spyder#13371

Paste Traceback/Error Below (if applicable)


PASTE TRACEBACK HERE

Versions

Dependencies

PASTE DEPENDENCIES HERE
dalthviz commented 2 years ago

Hi @dpturibio thank you for the feedback! I think this could be useful although we will need to check the different states of the toolbar buttons (probably disabled if nothing/multiple items are selected). What do you think @spyder-ide/core-developers ?

CAM-Gerlach commented 2 years ago

Seems like an okay idea to me, though depending on how long the list is and how big the window is, moving the mouse to the desired button may take longer than right clicking and doing the same. It also means they would need all be grayed out unless one and only one row was selected, since they are intrinsically properties of the selected item rather than the whole list, after all.

If the objective is just to make them quicker to access, @dpturibio how would you feel about adding a customizable keyboard shortcut to each one instead? It should be much easier to do (IIRC, just add one extra param to the call creating each action, and add defaults for each in the default config file), seems to more conceptually match the action types, avoids the issue of the state of the toolbar buttons needing to sync with the selections, and (most importantly) would be much faster for frequent users like yourselves than mousing all the way up to the toolbar and clicking a button, and you could set them to whatever you preferred.

dalthviz commented 2 years ago

Maybe we should move this to the UX/UI repo @ccordoba12 ?

ccordoba12 commented 2 years ago

Sure, please move it there.

CAM-Gerlach commented 2 years ago

I tried to move it, but it wouldn't let me; I suspect because unlike essentially all the other Spyder org repos, I don't have at least write-level access to that one.

dpturibio commented 2 years ago

@CAM-Gerlach and @dalthviz, thank you for your feedback. @dalthviz: I agree in disabling if nothing/multiple items are selected, depending of the action(in remove action, it is not necessary for multiple items for example, only for none). I can work on this checks for the toolbar. @CAM-Gerlach: regarding your questions on how long the list is and how big the window is, it won't be a problem as the window has a scroll bar and can be resized by the user accordingly to his needs. I checked with my team about the shortcuts, and they all prefer a static toolbar in the top of the window. It's visually more pleasing and functional, but we do not disagree in having also a shortcut. Please let me know what you think.

dalthviz commented 2 years ago

I would say that makes sense to me (to have shortcuts for the actions in the toolbar :+1: ). Also just in case, do you have any suggestions/ideas regarding this @isabela-pf ?

dpturibio commented 2 years ago

Hello guys, sorry for the long time without contacting. I was working on this improvement and I would like to share some code with you for suggestions and maybe having a direction to resolve some issues: @dalthviz, could you help me to mark the best person to give comments if is not you?

For the toolbar addition, I had to change two classes in spyder/widgets/collectionseditor.py in class BaseTableView(QTableView, SpyderConfigurationAccessor), method setup_menu, changed:

return menu

to

return [menu, menu_actions]

in order to have get the menu actions. And I suggest to change the method's name from setup_menu to setup_menu_and_toolbar

in class CollectionsEditorTableView(BaseTableView), init method, change:

self.menu = self.setup_menu()

to

self.menu, toolbar_items = self.setup_menu_and_toolbar()

and add the code:

self.toolbar = SpyderToolbar(parent = parent, title = 'Editor toolbar')
self.toolbar.setMinimumWidth(700)#necessary as it is not mainwindow, toolbar width is not resized
for item in toolbar_items:
   if item != None:
      self.toolbar.addAction(item)

the final result is: toolbarspyder

Question 1: how to auto-resize toolbar when not dealing with main window(for not need to use setMinimumWidth)? Any ideas? Question 2: how to accomodate toolbar in the window without overlapping the list?(as in the image)

dalthviz commented 2 years ago

Hi @dpturibio thank you for checking in again! I would say that probably @isabela-pf input about how this should look and work could be nice (also just in case anyone else has comments pinging @spyder-ide/core-developers ). Regarding your questions, I think the you will need to add the toolbar to the widget layout which is not in the CollectionsEditorTableView widget but in his parent (the CollectionsEditorWidget class at spyder/widgets/collectionseditor.py).

Also, regarding the change in the setup_menu method, I think you maybe will need to also do some changes in the RemoteCollectionsEditorTableView which also inherits from the BaseTableView and calls setup_menu.

Let us know if you have more comments or questions and thanks again for working on this!

dpturibio commented 2 years ago

Perfect observation @dalthviz. Adding the toolbar in CollectionsEditorWidget did the trick!

spyderToolbar

I will work on RemoteCollectionsEditorTableView class in order to handle the call to setup_menu method and share with you soon. Thanks a lot for the tips.

dpturibio commented 2 years ago

regarding setup_menu method of RemoteCollectionsEditorTableView class, could be modified as follows: from:

def setup_menu(self):

to

def setup_menu(self, setup_toolbar=False):

and then check

if setup_toolbar:
   return [menu, menu_actions]
else:
   return menu

what do you think @dalthviz ? If you are ok with these changes, let me know if I can work on a merge request for it. Thank you.

CAM-Gerlach commented 2 years ago

As a side note, changing the return type like this is generally a practice that is often best avoided and sometimes considered an antipattern, but that ultimately comes down to the current conventions and patterns in the Spyder API, which @dalthviz and the others would be better informed on than I.

dalthviz commented 2 years ago

Hi again @dpturibio ! Thinking about the setup_menu maybe we could save the actions in a class attribute instead of changing the method return structure. So, something like:

def setup_menu(self):
    ...
    self.menu_actions = menu_actions
    return menu

And then we would be able to get the menu actions from the RemoteCollectionsEditorTableView or CollectionsEditorTableView on the CollectionsEditorWidget with something like:


self.editor.menu_actions

In any case, if you have already some code you can make a PR :) Then we can check the functionality and suggest or comment on the code directly. Let us know what do you think!

Also thanks @CAM-Gerlach for the feedback :+1:

dpturibio commented 2 years ago

Thank you for the contributions @dalthviz @CAM-Gerlach Just a last question: which branch should I derive the code from?

dalthviz commented 2 years ago

No problem @dpturibio and I think that probably you should use the 5.x branch, but just in case pinging @ccordoba12 to double check

dpturibio commented 2 years ago

Thank you @dalthviz for the guidance. I submitted the PR as you can see above. What is the workflow from now on? Let me know if I need to take some further action.

dalthviz commented 2 years ago

Glad to help @dpturibio and now I think we could continue the discussion over the PR (so we can check the actual code and give you feedback over there). Could you request me as a reviewer in the PR you opened to not forget to check it please? Thanks for your interest and work on this one!