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

wxGUI/dbmgr: fix removing map table layer and layer related Browse data and Manage tables page (tab) #2422

Merged
merged 5 commits into from
Oct 3, 2023

Conversation

tmszi
Copy link
Member

@tmszi tmszi commented Jun 9, 2022

Describe the bug
Removing map table layer don't work and related layer page (tab) on the Browse data and Manage tables page (tab) aren't deleted too.

To Reproduce
Steps to reproduce the behavior:

  1. Launch Attribute Table Manager e.g g.gui.dbmgr geology
  2. Switch to Manage layers page (tab)
  3. Create new table with name e.g. test (right side on the Add layer page (tab))
  4. Add new layer with layer number 2, and choose test table from table choice widget and hit Add layer button
  5. On the Manage layers page (tab) switch to Remove layer page (tab)
  6. Choose layer 2 from Layer to remove ComboBox widget choices and hit Remove layer button
  7. See error
Traceback (most recent call last):
  File "/usr/lib64/grass83/gui/wxpython/dbmgr/base.py", line 3815, in OnDeleteLayer
    self.parentDialog.parentDbMgrBase.UpdateDialog(layer=layer)
  File "/usr/lib64/grass83/gui/wxpython/dbmgr/manager.py", line 253, in UpdateDialog
    DbMgrBase.UpdateDialog(self, layer=layer)
  File "/usr/lib64/grass83/gui/wxpython/dbmgr/base.py", line 860, in UpdateDialog
    self.pages["browse"].DeletePage(layer)
  File "/usr/lib64/grass83/gui/wxpython/dbmgr/base.py", line 1037, in DeletePage
    self.selLayer = self.layers[self.GetSelection()]
IndexError: list index out of range

Expected behavior
Removing map table layer should be work without error message, and related layer page (tab) on the Browse data and Manage tables page (tab) be deleted too.

System description (please complete the following information):

  • Operating System: all
  • GRASS GIS version: all

@tmszi tmszi added bug Something isn't working backport_needed GUI wxGUI related labels Jun 9, 2022
@tmszi tmszi added this to the 8.2.1 milestone Jun 9, 2022
@@ -1018,7 +1018,7 @@ def DeletePage(self, layer):
if layer not in self.layers:
return False

GNotebook.DeleteNBPage(self, self.layers.index(layer))
GNotebook.DeletePage(self, self.layers.index(layer))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please comment on this more? The new code uses directly the method of the FlatNotebook, but why? Is something wrong about GNotebook's implementation? Wouldn't it be possibly better to adjust the code of this function to work with the GNotebook's version of DeletNBPage?

Copy link
Member Author

@tmszi tmszi Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotebookController base class, InsertPage method has 2 way how to insert page:

def InsertPage(self, *args, **kwargs):
"""Insert a new page"""
if "name" in kwargs:
self.notebookPages[kwargs["name"]] = kwargs["page"]
del kwargs["name"]
try:
self.classObject.InsertPage(self.widget, *args, **kwargs)
except TypeError as e: # documentation says 'index', but certain versions of wx require 'n'
kwargs["n"] = kwargs["index"]
del kwargs["index"]
self.classObject.InsertPage(self.widget, *args, **kwargs)

  1. Insert page with name param
  2. Insert page without name param (this is preferred way in Attribute Table Manager code)
    self.InsertNBPage(
    index=pos,
    page=panel,
    text=" %d / %s %s"
    % (layer, label, self.dbMgrData["mapDBInfo"].layers[layer]["table"]),
    )

NotebookController base class, inside DeletePage method is called method GetPageIndexByName which is not return page index (return -1) if page was inserted without name param.

def DeletePage(self, page):
"""Delete page
:param page: name
:return: True if page was deleted, False if not exists
"""
delPageIndex = self.GetPageIndexByName(page)
if delPageIndex != -1:
ret = self.classObject.DeletePage(self.widget, delPageIndex)
if ret:
del self.notebookPages[page]
return ret
else:
return False

FlatNotebookController class, GetPageIndexByName method

def GetPageIndexByName(self, page):
"""Get notebook page index
:param page: name
"""
if page not in self.notebookPages:
return -1

GNotebook class, method DeleteNBPage implementation allow delete page which was inserted with name param only.

@neteler neteler modified the milestones: 8.2.1, 8.2.2 Jan 22, 2023
@wenzeslaus wenzeslaus modified the milestones: 8.2.2, 8.3.1 Jun 6, 2023
@tmszi tmszi force-pushed the wxgui_dbmgr_fix_remove_layer branch from a4419dd to 327d334 Compare September 27, 2023 06:39
@tmszi
Copy link
Member Author

tmszi commented Sep 27, 2023

Rebase 327d334.

@tmszi
Copy link
Member Author

tmszi commented Sep 30, 2023

@petrasovaa I refactored the use of InsertNBPage() (method use name param arg) method inside wxGUI/dbmgr component. Now DeleteNBPage() works as expected. Tested succesfully as usual.

@tmszi
Copy link
Member Author

tmszi commented Sep 30, 2023

Wouldn't it be possibly better to adjust the code of this function to work with the GNotebook's version of DeletNBPage?

With 086b5b2 I refactor GetPageIndexByName() method of FlatNotebookController and of NotebookController class to meet your suggention. It is now possible to delete a notebook page by name or by page index position depending on how the page was added or inserted.

Copy link
Contributor

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I think this is better!

@tmszi tmszi merged commit df6e6ae into OSGeo:main Oct 3, 2023
18 checks passed
tmszi added a commit to tmszi/grass that referenced this pull request Oct 3, 2023
tmszi added a commit to tmszi/grass that referenced this pull request Oct 3, 2023
@tmszi tmszi deleted the wxgui_dbmgr_fix_remove_layer branch October 3, 2023 19:08
landam pushed a commit to landam/grass that referenced this pull request Oct 25, 2023
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
HuidaeCho pushed a commit to HuidaeCho/grass that referenced this pull request Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working GUI wxGUI related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants