Add more arguments for edit string popups

[MattRoyle]

Fixes #132. Adds arguments for the window style and Cancel and Done buttons.
Remove unnecessary call to __request_close_fn in reject that caused a console error when clicking escape.
If either button is a string then it will display the buttons, otherwise it will have the existing behavior of not having buttons.

[cmeyer]

On macOS, it looks like this.

On Windows, it looks like this.

On both platforms, the Escape key no longer works - as expected since the self.__request_close_fn() was removed.

Tested from console with this script:

from nion.ui import Dialog
Dialog.pose_edit_string_pop_up("X", lambda x: None, window=_window)

I would recommend:

• switch back to the old style window that does not have close/min/max buttons.
• model it after .Net MessageBox function
• restore the escape handling.

I cannot reproduce the escape-triggering-console message problem - so let's try to reproduce that reliably and fix it separately before this PR.

[MattRoyle]

On both platforms, the Escape key no longer works - as expected since the self.__request_close_fn() was removed.

Tested from console with this script:

from nion.ui import Dialog
Dialog.pose_edit_string_pop_up("X", lambda x: None, window=_window)

The script you ran hadn't set the window_style so it was defaulting to a window vs if you pass in a "default" it will be the modeless style. The modeless style would throw the console error when return was pressed, while the window would behave properly. I updated the code to check that the window style is not "default" before running the close function which means the escape now should work again in your tests. If there are any other dialog options that cause that error, i.e. that don't make a window, then it should check those too, but I haven't done that yet.

[MattRoyle]

@cmeyer I removed the windows_style parameter so now it will be a popup by default.

[MattRoyle]

To explain the window style changes:
nionui Application.cpp line 2496 has three window types:

mapping["dialog"] = Qt::Dialog;
mapping["popup"] = Qt::Popup;
mapping["tool"] = Qt::Tool;

and some hints

mapping["floating-hint"] = Qt::WindowStaysOnTopHint;
mapping["frameless-hint"] = Qt::FramelessWindowHint;
mapping["title-hint"] = Qt::WindowTitleHint;
mapping["customize-hint"] = Qt::CustomizeWindowHint;
mapping["close-button-hint"] = Qt::WindowCloseButtonHint;
mapping["min-button-hint"] = Qt::WindowMinimizeButtonHint;
mapping["max-button-hint"] = Qt::WindowMaximizeButtonHint;
mapping["system-menu-hint"] = Qt::WindowSystemMenuHint;
mapping["help-hint"] = Qt::WindowContextHelpButtonHint;
mapping["fullscreen-hint"] = Qt::WindowFullscreenButtonHint;
mapping["input-transparent"] = Qt::WindowTransparentForInput;
mapping["no-focus"] = Qt::WindowDoesNotAcceptFocus;

Previously pose_edit_string_popup was initialising the popup with None which would be a regular window, but it then would set the style to "tool", "frameless-hint".

This is because of is explained by the comment

# passing window_style=None is essential for making copy and paste work, as the 'popup' style does not handle copy/paste.

But this is no longer the case so instead the popup can just be "popup".

[cmeyer]

A few quick points...

The reject function in pose_confirmation_pop_up has the same bug that was fixed in this PR in pose_edit_string_pop_up: the extra call to __request_close_fn. The pose_select_item_pop_up is already correct. That also points to a quick change or follow-up maintenance PR: pull the common Handler base out of each of those functions so that the common code is only written once.

Also, the arguments to button text arguments to pose_confirmation_pop_up should be None rather than hard coded strings. Making that change will match how pose_edit_string_pop_up handles it.

[cmeyer]

The same Handler code might also appear here, albeit in more complicated form: https://github.com/nion-software/nionswift/blob/2cde63c489a40de54dfab33034069abf4c281da8/nion/swift/DisplayEditPopup.py#L131. Can you also give that code a look to see how if it is similar and correct?

[MattRoyle]

The same Handler code might also appear here, albeit in more complicated form: https://github.com/nion-software/nionswift/blob/2cde63c489a40de54dfab33034069abf4c281da8/nion/swift/DisplayEditPopup.py#L131. Can you also give that code a look to see how if it is similar and correct?

There are many similar Handler code blocks throughout swift, but consolidating them should be a separate project. The handler in the pose_confirmation popup is intentionally different from pose_edit_string_popup. It allows for escape to trigger closing but does NOT allow enter to confirm. That limitation was made as I implemented it for removing a workspace and thought it shouldn't be able to click enter to do that, but maybe it should be consistent.

[MattRoyle]

The reject function in pose_confirmation_pop_up has the same bug that was fixed in this PR in pose_edit_string_pop_up: the extra call to __request_close_fn.

It doesn't present this behavior I think because the init_popup function that sets the request close fn is never called, but this was an unintentional fix so I'll change it to the same fix as the other pose_edit_string_popup handler.

Also, the arguments to button text arguments to pose_confirmation_pop_up should be None rather than hard coded strings. Making that change will match how pose_edit_string_pop_up handles it.

Will do!

[MattRoyle]

The reject function in pose_confirmation_pop_up has the same bug that was fixed in this PR in pose_edit_string_pop_up: the extra call to __request_close_fn.

Because it was a "popup" window it doesn't extra call. I removed the window_style from the arguments so it is always a "popup".

[cmeyer]

I think we should change the name of all of the functions to use popup instead of pop_up. To maintain backwards compatibility, we can have lines at the bottom of the file (with a compatibility comment) like:

pose_edit_string_pop_up = pose_edit_string_popup
pose_select_item_pop_up = pose_select_item_popup

There is no need for a pose_confirmation_pop_up line as it is just being introduced with this PR.

[MattRoyle]

The position is now defaults based off the arguments passed into the function:

"""
If the position is not None then the popup will be centered on that point.
Otherwise, the popup will use get_popup_position to be centered on the top third of the parent_rect parameter \ 
if it is not None or the window if parent_rect is None.
"""

[cmeyer]

From @Ion-e

it changes the popup dialog used in the renaming of collections

I think we're going to need something more sophisticated for window positioning - or at least able to address the positioning requirements that the top-left of the dialog appears at a particular point (see CollectionsWidget in nionswift).

[cmeyer]

Merging with cleanup, ebb47f0

diff --git a/nion/ui/Dialog.py b/nion/ui/Dialog.py
index 7bc447e..3218409 100644
--- a/nion/ui/Dialog.py
+++ b/nion/ui/Dialog.py
@@ -11,8 +11,6 @@ import time
 import typing
 import weakref
 
-from fontTools.subset import prune_hints
-
 # third party libraries
 # none
 
@@ -243,12 +241,16 @@ class PopupWindow(Window.Window):
         super().close()
 
 
-def pose_select_item_pop_up(items: typing.Sequence[typing.Any], completion_fn: typing.Callable[[typing.Any], None], *,
-                            window: Window.Window, current_item: int = 0,
-                            item_getter: typing.Callable[[typing.Any], str] | None = None,
-                            title: str | None = None, position: Geometry.IntPoint | None = None,
-                            size: Geometry.IntSize | None = None, parent_rect: Geometry.IntRect | None = None,
-                            position_offset: Geometry.IntSize | Geometry.FloatSize | None = None) -> None:
+def pose_select_item_popup(items: typing.Sequence[typing.Any],
+                           completion_fn: typing.Callable[[typing.Any], None], *,
+                           window: Window.Window,
+                           title: str | None = None,
+                           current_item: int = 0,
+                           item_getter: typing.Callable[[typing.Any], str] | None = None,
+                           position: Geometry.IntPoint | None = None,
+                           size: Geometry.IntSize | None = None,
+                           parent_rect: Geometry.IntRect | None = None,
+                           position_offset: Geometry.IntSize | Geometry.FloatSize | None = None) -> None:
     """Create a popup that allows the user to select an item from a list.
 
     The popup's position will depend on the passed parameters.
@@ -283,8 +285,8 @@ def pose_select_item_pop_up(items: typing.Sequence[typing.Any], completion_fn: t
 
         def accept(self, widget: UserInterface.Widget) -> bool:
             # receive this when the user hits return. need to request a close and return True to say we handled event.
-            self.__request_close_fn()
             self.is_rejected = False
+            self.__request_close_fn()
             return True
 
         def handle_select(self, widget: UserInterface.Widget) -> None:
@@ -299,7 +301,7 @@ def pose_select_item_pop_up(items: typing.Sequence[typing.Any], completion_fn: t
     # calculate the max string width, add 10%, min 200, max 480
     width = min(max(int(max([window.get_font_metrics("system", item_getter(c)).width for c in items]) * 1.10), 200), 480)
 
-    size, position = _get_pop_up_size_and_position(window, position=position, size=size, parent_rect=parent_rect, position_offset=position_offset)
+    size, position = _get_popup_size_and_position(window, position=position, size=size, parent_rect=parent_rect, position_offset=position_offset)
 
     ui_handler = Handler()
     u = Declarative.DeclarativeUI()
@@ -322,11 +324,17 @@ def pose_select_item_pop_up(items: typing.Sequence[typing.Any], completion_fn: t
     popup.show(position=position, size=size)
 
 
-def pose_edit_string_pop_up(current_string: str, completion_fn: typing.Callable[[str | None], None], *,
-                            window: Window.Window, title: str | None = None,
-                            position: Geometry.IntPoint | None = None, size: Geometry.IntSize | None = None,
-                            cancel_button_text: str | None = None, accept_button_text: str | None = None, show_buttons: bool = False,
-                            parent_rect: Geometry.IntRect | None = None, position_offset: Geometry.IntSize | Geometry.FloatSize | None = None) -> None:
+def pose_edit_string_popup(current_string: str,
+                           completion_fn: typing.Callable[[str | None], None], *,
+                           window: Window.Window,
+                           title: str | None = None,
+                           position: Geometry.IntPoint | None = None,
+                           size: Geometry.IntSize | None = None,
+                           parent_rect: Geometry.IntRect | None = None,
+                           position_offset: Geometry.IntSize | Geometry.FloatSize | None = None,
+                           cancel_button_text: str | None = None,
+                           accept_button_text: str | None = None,
+                           show_buttons: bool = False) -> None:
     """ Create a popup with a text input field.
 
     The popup's position will depend on the passed parameters.
@@ -373,9 +381,8 @@ def pose_edit_string_pop_up(current_string: str, completion_fn: typing.Callable[
             self.__request_close_fn()
 
     from nion.ui import Declarative  # avoid circular reference
-    size, position = _get_pop_up_size_and_position(window, position=position, size=size, parent_rect=parent_rect, position_offset=position_offset)
-    # calculate the max string width, add 10%, min 200, max 480
-    width = (size.width - 20) if size else min(max(int(window.get_font_metrics("system", current_string).width * 1.10), 200), 480)
+    size, position = _get_popup_size_and_position(window, position=position, size=size, parent_rect=parent_rect, position_offset=position_offset)
+    width = size.width - 20
 
     ui_handler = Handler(current_string)
     u = Declarative.DeclarativeUI()
@@ -387,9 +394,9 @@ def pose_edit_string_pop_up(current_string: str, completion_fn: typing.Callable[
         accept_button_text = accept_button_text or _("Done")
         button_row = u.create_row(u.create_stretch(),
                                   u.create_push_button(text=cancel_button_text, on_clicked="handle_cancel"),
-                                  u.create_push_button(text=accept_button_text, on_clicked="accept"), spacing=8, margin=8)
+                                  u.create_push_button(text=accept_button_text, on_clicked="accept"),
+                                  spacing=8, margin=8)
         column = u.create_column(column, button_row)
-    # Passing window_style='popup' previously did not handle copy/paste, this issue seems to be resolved.
     popup = PopupWindow(window, column, ui_handler, window_style='popup', delegate=ui_handler)
 
     def handle_close(old_close: typing.Callable[[], None] | None) -> None:
@@ -409,12 +416,18 @@ def pose_edit_string_pop_up(current_string: str, completion_fn: typing.Callable[
     line_edit_widget.focused = True
 
 
-def pose_confirmation_pop_up(completion_fn: typing.Callable[[bool], None], *,
-                             window: Window.Window, title: str | None = None, caption: str | None = None,
-                             position: Geometry.IntPoint | None = None, size: Geometry.IntSize | None = None,
-                             cancel_button_text: str | None = None, accept_button_text: str | None = None,
-                             show_buttons: bool = False, parent_rect: Geometry.IntRect | None = None,
-                             position_offset: Geometry.IntSize | Geometry.FloatSize | None = None, show_cancel_button: bool = True) -> None:
+def pose_confirmation_popup(completion_fn: typing.Callable[[bool], None], *,
+                            window: Window.Window,
+                            title: str | None = None,
+                            caption: str | None = None,
+                            position: Geometry.IntPoint | None = None,
+                            size: Geometry.IntSize | None = None,
+                            parent_rect: Geometry.IntRect | None = None,
+                            position_offset: Geometry.IntSize | Geometry.FloatSize | None = None,
+                            cancel_button_text: str | None = None,
+                            accept_button_text: str | None = None,
+                            show_buttons: bool = False,
+                            show_cancel_button: bool = True) -> None:
     """ Display a confirmation popup.
 
     show_cancel_button parameter is for when show_buttons is True, but you only want to show the accept button.
@@ -451,8 +464,7 @@ def pose_confirmation_pop_up(completion_fn: typing.Callable[[bool], None], *,
 
     from nion.ui import Declarative  # avoid circular reference
 
-    # calculate the max string width, add 10%, min 200, max 480
-    size, position = _get_pop_up_size_and_position(window, position=position, size=size, parent_rect=parent_rect, position_offset=position_offset)
+    size, position = _get_popup_size_and_position(window, position=position, size=size, parent_rect=parent_rect, position_offset=position_offset)
 
     ui_handler = Handler()
     u = Declarative.DeclarativeUI()
@@ -465,10 +477,12 @@ def pose_confirmation_pop_up(completion_fn: typing.Callable[[bool], None], *,
         if show_cancel_button:
             button_row = u.create_row(u.create_stretch(),
                                       u.create_push_button(text=cancel_button_text, on_clicked="handle_cancel"),
-                                      u.create_push_button(text=accept_button_text, on_clicked="accept"), spacing=8, margin=8)
+                                      u.create_push_button(text=accept_button_text, on_clicked="accept"),
+                                      spacing=8, margin=8)
         else:
             button_row = u.create_row(u.create_stretch(),
-                                     u.create_push_button(text=accept_button_text, on_clicked="accept"), spacing=8, margin=8)
+                                      u.create_push_button(text=accept_button_text, on_clicked="accept"),
+                                      spacing=8, margin=8)
 
     caption_row = None
     if caption:
@@ -506,10 +520,11 @@ def _get_popup_position_from_parent_rect(parent_panel_rect: Geometry.IntRect, po
     return Geometry.IntPoint(x=parent_panel_rect.left + (parent_panel_rect.width - popup_size.width) // 2, y=vertical_position)
 
 
-def _get_pop_up_size_and_position(window: Window.Window, position: Geometry.IntPoint | None = None,
-                                  size: Geometry.IntSize | None = None, parent_rect: Geometry.IntRect | None = None,
-                                  position_offset: Geometry.IntSize | Geometry.FloatSize | None = None)\
-        -> tuple[Geometry.IntSize, Geometry.IntPoint]:
+def _get_popup_size_and_position(window: Window.Window,
+                                 position: Geometry.IntPoint | None = None,
+                                 size: Geometry.IntSize | None = None,
+                                 parent_rect: Geometry.IntRect | None = None,
+                                 position_offset: Geometry.IntSize | Geometry.FloatSize | None = None) -> tuple[Geometry.IntSize, Geometry.IntPoint]:
     """ Calculate the size and position of a popup.
 
     The popup's position will depend on the passed parameters.
@@ -520,7 +535,6 @@ def _get_pop_up_size_and_position(window: Window.Window, position: Geometry.IntP
     Passing position_offset a FloatSize will be treated as a percentage of the popup size to offset from the top left position.
     """
     size = size or Geometry.IntSize(width=400, height=100)
-    assert size is not None
 
     if position is None:
         if parent_rect is None:
@@ -530,10 +544,14 @@ def _get_pop_up_size_and_position(window: Window.Window, position: Geometry.IntP
         assert parent_rect is not None
         position = _get_popup_position_from_parent_rect(parent_rect, size)
 
-    if position_offset is not None:  # Offset is CSS standard, meaning positive values are down, right
+    if position_offset is not None:
         if isinstance(position_offset, Geometry.IntSize):
             position = Geometry.IntPoint(position.y + position_offset.height, position.x + position_offset.width)
         elif isinstance(position_offset, Geometry.FloatSize):
             position = Geometry.IntPoint(position.y + int(size.height * position_offset.height), position.x + int(size.width * position_offset.width))
 
     return size, position
+
+
+pose_edit_string_pop_up = pose_edit_string_popup
+pose_select_item_pop_up = pose_select_item_popup