wxGUI/main_window: fix close map notebook page #2072

tmszi · 2022-01-07T07:50:56Z

Describe the bug
Closing the map notebook page ends with an error message.

To Reproduce
Steps to reproduce the behavior:

Launch wxGUI with single window mode
Try close map notebook page
See error

Traceback (most recent call last):
  File "/home/tomas/.local/lib/python3.9/site-
packages/wx/core.py", line 3285, in <lambda>

lambda event: event.callable(*event.args, **event.kw) )
  File "/home/tomas/.local/lib/python3.9/site-
packages/wx/lib/agw/aui/auibook.py", line 3685, in
DeletePage

wnd.Destroy()
wx._core
.
wxAssertionError
:
C++ assertion "GetEventHandler() == this" failed at
/tmp/pip-req-build-
oxkmg2wi/ext/wxWidgets/src/common/wincmn.cpp(477) in
~wxWindowBase(): any pushed event handlers must have been
removed

Expected behavior
Closing the map notebook page should work without an error message.

System description:

GRASS GIS version main git branch

Additional context
Only in single window mode.

Only in single window mode.

petrasovaa · 2022-01-07T19:04:55Z

There is also #1789.

lindakarlovska

I tried to directly push my changes into your PR @tmszi but it did not work, so I am putting it here. You can test it, when you switch map notebook tabs and then close the given map notebook tab it should close the right display tab - for me it works good.

I would place pgnum_dict (cause map display index can be different from display tab index):

main_window/frame.py:

def CanCloseDisplay(askIfSaveWorkspace):
  """Callback to check if user wants to close display.
  Map Display index can be different from index in Display tab."""
  pgnum_dict = {}
  pgnum_dict["display"] = self.notebookLayers.GetPageIndex(page)
  pgnum_dict["mapnotebook"] = self.mapnotebook.GetPageIndex(mapdisplay)
  name = self.notebookLayers.GetPageText(pgnum_dict["display"])
  caption = _("Close Map Display {}").format(name)
  if not askIfSaveWorkspace or (
      askIfSaveWorkspace and self.workspace_manager.CanClosePage(caption)
  ):
      return pgnum_dict
  return None

def _closePageNoEvent(self, pgnum_dict):
    """Close page and destroy map display without
    generating notebook page closing event"""
    self.notebookLayers.Unbind(FN.EVT_FLATNOTEBOOK_PAGE_CLOSING)
    self.notebookLayers.DeletePage(pgnum_dict["display"])
    self.notebookLayers.Bind(FN.EVT_FLATNOTEBOOK_PAGE_CLOSING, self.OnCBPageClosing)
    self.mapnotebook.DeletePage(pgnum_dict["mapnotebook"])

mapdisp/frame.py:

def OnCloseWindow(self, event, askIfSaveWorkspace=True):
  """Window closed.
  Also close associated layer tree page
  """
  Debug.msg(2, "MapPanel.OnCloseWindow()")
  if self.canCloseDisplayCallback:
      pgnum_dict = self.canCloseDisplayCallback(askIfSaveWorkspace=askIfSaveWorkspace)
      if pgnum_dict["display"] is not None:
          self.CleanUp()
            if pgnum_dict["display"] > -1:
                self.closingDisplay.emit(pgnum_dict=pgnum_dict)
                # Destroy is called when notebook page is deleted
    else:
        self.CleanUp()
        self.Destroy()

lindakarlovska · 2022-01-07T20:13:11Z

There is also #1789.

That's the first thing. The second one is the switching of map notebook pages. I've shared my code repairing it. Btw @tmszi you've rescued me :-) because I got pretty lost in #1789. It is indeed unnecessarily complicated.

lindakarlovska · 2022-01-07T20:16:44Z

I have tested the Close current mapdisplay and Close all mapdisplays from Settings and it works also good after above-shared changes. @tmszi please could you also update a list of shorcuts so that the map notebook page can be also closed by CTRL+W shortcut?

To avoid incorrectly closing the map notebook page, if the pages of the map notebook change position by dragging and dropping.

tmszi · 2022-01-07T21:24:07Z

I tried to directly push my changes into your PR @tmszi but it did not work, so I am putting it here. You can test it, when you switch map notebook tabs and then close the given map notebook tab it should close the right display tab - for me it works good.

All right, I've applied and tested your changes, and it's working as expected :-).

I would place pgnum_dict (cause map display index can be different from display tab index).

Yes, you're right.

tmszi · 2022-01-07T21:27:49Z

There is also #1789.

That's the first thing. The second one is the switching of map notebook pages. I've shared my code repairing it. Btw @tmszi you've rescued me :-) because I got pretty lost in #1789. It is indeed unnecessarily complicated.

You are welcome, so together we simplified the code :-).

When the map display notebook page has a vector/raster map displayed and the page is closed, and pressed the Cancel button in the Close Map Display dialog, canCloseDisplayCallback function return None not dict value.

tmszi · 2022-01-07T21:45:45Z

I have tested the Close current mapdisplay and Close all mapdisplays from Settings and it works also good after above-shared changes. @tmszi please could you also update a list of shorcuts so that the map notebook page can be also closed by CTRL+W shortcut?

The keyboard shortcut Ctrl+W (close map notebook page) works for me without any changes.

lindakarlovska · 2022-01-08T08:05:48Z

There is also #1789.

It is one thing and the second one is the problem when you switch map notebook tabs.
But @tmszi you rescue me :-) because I got a bit lost in PR #1789. It is unnecessarily complicated.

I tried to incorporate some changes that repair closing when someone switch map notebook pages. Feel free to comment. I think that the whole concept of closing events is a bit strange (we talked about it with @petrasovaa a longer time ago but actually I am not sure what was the result?).

I have tested the Close current mapdisplay and Close all mapdisplays from Settings and it works also good after above-shared changes. @tmszi please could you also update a list of shorcuts so that the map notebook page can be also closed by CTRL+W shortcut?

The keyboard shortcut Ctrl+W (close map notebook page) works for me without any changes.

Right, I have tested all functionalities - normal closing, closing from settings, and CTRL+W shortcut and all works for me as expected :-)

lindakarlovska

I just realize that I have used outdated terminology, still living in the time when the Layers tab was called Display, even though it was my own idea to rename the Display back to Layers :-) Please @tmszi could you just replace "Display" with "Layers"? Sorry for the confusion. :-)

…ored in the dict Fixes closing the map display notebook layers tree page in multi window mode.

tmszi · 2022-01-08T08:50:49Z

This commit ce0586d solve bug with closing map display notebook layers tree page in multi window mode.

tmszi · 2022-01-08T08:56:00Z

I just realize that I have used outdated terminology, still living in the time when the Layers tab was called Display, even though it was my own idea to rename the Display back to Layers :-) Please @tmszi could you just replace "Display" with "Layers"? Sorry for the confusion. :-)

Yes I could. Do you mean name of layers notebook page and name of the map display notebook page (e.g. Map Display 1 -> Map Layers 1)?

Default:

Expected:

lindakarlovska · 2022-01-08T11:28:22Z

I just realize that I have used outdated terminology, still living in the time when the Layers tab was called Display, even though it was my own idea to rename the Display back to Layers :-) Please @tmszi could you just replace "Display" with "Layers"? Sorry for the confusion. :-)

Yes I could. Do you mean name of layers notebook page and name of the map display notebook page (e.g. Map Display 1 -> Map Layers 1)?

Default:

Expected:

No, no, sorry, I did not express well, I meant just to have the terminology correct according to #1927. So, in the code to change the Display tab name to Layers tab name, e.g. pgnum_dict["layers"]. But it is a detail, if you prefer to let it as it is e.g. pgnum_dict["display"], it is okay. :-)

lindakarlovska · 2022-01-08T11:33:50Z

I just realize that I have used outdated terminology, still living in the time when the Layers tab was called Display, even though it was my own idea to rename the Display back to Layers :-) Please @tmszi could you just replace "Display" with "Layers"? Sorry for the confusion. :-)

Yes I could. Do you mean name of layers notebook page and name of the map display notebook page (e.g. Map Display 1 -> Map Layers 1)?
Default:

Expected:

No, no, sorry, I did not express well, I meant just to have the terminology correct according to #1927. So, in the code to change the Display tab name to Layers tab name, e.g. pgnum_dict["layers"]. But it is a detail, if you prefer to let it as it is e.g. pgnum_dict["display"], it is okay. :-)

I have looked that you put a good explanation of the dictionary into the docs, so you can let it as it is probably. :-)

tmszi · 2022-01-08T12:58:00Z

No, no, sorry, I did not express well, I meant just to have the terminology correct according to #1927. So, in the code to change the Display tab name to Layers tab name, e.g. pgnum_dict["layers"]. But it is a detail, if you prefer to let it as it is e.g. pgnum_dict["display"], it is okay. :-)

Yes, you're right. I changed the display dictionary key name to layers in this commit fee16af.

Only in single window mode. * wxGUI/main_window: fix getting correct map notebook page index To avoid incorrectly closing the map notebook page, if the pages of the map notebook change position by dragging and dropping. * wxGUI/mapdisp: fix checking pgnum_dict variable if it's None When the map display notebook page has a vector/raster map displayed and the page is closed, and pressed the Cancel button in the Close Map Display dialog, canCloseDisplayCallback function return None not dict value. * wxGUI/lmgr: actual closed map display notebook layers tree page is stored in the dict Fixes closing the map display notebook layers tree page in multi window mode. Co-authored-by: Linda Kladivova <[email protected]>

wxGUI/main_window: fix close map notebook page

9ba506b

Only in single window mode.

tmszi added bug Something isn't working GUI wxGUI related labels Jan 7, 2022

tmszi added this to the 8.2.0 milestone Jan 7, 2022

tmszi requested a review from lindakarlovska January 7, 2022 07:50

lindakarlovska requested changes Jan 7, 2022

View reviewed changes

wxGUI/main_window: fix getting correct map notebook page index

f8226e9

To avoid incorrectly closing the map notebook page, if the pages of the map notebook change position by dragging and dropping.

wxGUI/mapdisp: fix checking pgnum_dict variable if it's None

9307acb

When the map display notebook page has a vector/raster map displayed and the page is closed, and pressed the Cancel button in the Close Map Display dialog, canCloseDisplayCallback function return None not dict value.

lindakarlovska requested changes Jan 8, 2022

View reviewed changes

wxGUI/lmgr: actual closed map display notebook layers tree page is st…

ce0586d

…ored in the dict Fixes closing the map display notebook layers tree page in multi window mode.

Fix class methods docs

0c35e03

Change dict var pgnum_dict "display" key name to "layers"

fee16af

Fix class methods docs

97be1dd

lindakarlovska approved these changes Jan 8, 2022

View reviewed changes

Fix class methods docs

5b247b0

tmszi merged commit 0f76588 into OSGeo:main Jan 9, 2022

tmszi deleted the fix-wxgui-single-layout-close-map-display-page branch January 9, 2022 05:17

lindakarlovska mentioned this pull request Jan 13, 2022

wxGUI/Single-Window: New close page event for AuiNotebook #1789

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

wxGUI/main_window: fix close map notebook page #2072

wxGUI/main_window: fix close map notebook page #2072

Uh oh!

tmszi commented Jan 7, 2022

Uh oh!

petrasovaa commented Jan 7, 2022

Uh oh!

lindakarlovska left a comment •

edited

Loading

Uh oh!

lindakarlovska commented Jan 7, 2022

Uh oh!

lindakarlovska commented Jan 7, 2022

Uh oh!

tmszi commented Jan 7, 2022

Uh oh!

tmszi commented Jan 7, 2022

Uh oh!

tmszi commented Jan 7, 2022

Uh oh!

lindakarlovska commented Jan 8, 2022

Uh oh!

lindakarlovska left a comment

Uh oh!

tmszi commented Jan 8, 2022

Uh oh!

tmszi commented Jan 8, 2022 •

edited

Loading

Uh oh!

lindakarlovska commented Jan 8, 2022

Uh oh!

lindakarlovska commented Jan 8, 2022

Uh oh!

tmszi commented Jan 8, 2022

Uh oh!

Uh oh!

Uh oh!

wxGUI/main_window: fix close map notebook page #2072

wxGUI/main_window: fix close map notebook page #2072

Uh oh!

Conversation

tmszi commented Jan 7, 2022

Uh oh!

petrasovaa commented Jan 7, 2022

Uh oh!

lindakarlovska left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lindakarlovska commented Jan 7, 2022

Uh oh!

lindakarlovska commented Jan 7, 2022

Uh oh!

tmszi commented Jan 7, 2022

Uh oh!

tmszi commented Jan 7, 2022

Uh oh!

tmszi commented Jan 7, 2022

Uh oh!

lindakarlovska commented Jan 8, 2022

Uh oh!

lindakarlovska left a comment

Choose a reason for hiding this comment

Uh oh!

tmszi commented Jan 8, 2022

Uh oh!

tmszi commented Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lindakarlovska commented Jan 8, 2022

Uh oh!

lindakarlovska commented Jan 8, 2022

Uh oh!

tmszi commented Jan 8, 2022

Uh oh!

Uh oh!

lindakarlovska left a comment •

edited

Loading

tmszi commented Jan 8, 2022 •

edited

Loading