Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
7198ede
Check if plugin manager is busy on close
goanpeca Jun 17, 2024
433d298
Fix typing
goanpeca Jun 17, 2024
2a412dd
Fix test
goanpeca Jun 17, 2024
3784987
Add process status manager
goanpeca Jul 29, 2024
3711c4c
Fix typing
goanpeca Jul 29, 2024
cd5bb9c
Update API to include callbkacs and history of updates
goanpeca Jul 30, 2024
fe708c5
Merge branch 'main' into enh/check-plugin-manager-on-close
dalthviz Aug 13, 2025
61643a4
Fix typing
dalthviz Aug 13, 2025
db40eb7
Merge branch 'main' into enh/check-plugin-manager-on-close
dalthviz Aug 18, 2025
44192f5
Update logic to trigger cancel tasks on close if needed
dalthviz Aug 18, 2025
11498c7
Fix typing
dalthviz Aug 18, 2025
dfa3c92
Add cancelled status. Add a test
dalthviz Aug 19, 2025
619a6b5
Add comment about dict usage to hold task status information and conc…
dalthviz Aug 26, 2025
5b01321
Merge branch 'main' into enh/check-plugin-manager-on-close
dalthviz Aug 27, 2025
38e4905
Update Status enum values and some docstrings
dalthviz Aug 28, 2025
12fb26b
Merge branch 'enh/check-plugin-manager-on-close' of https://github.co…
dalthviz Aug 28, 2025
45a54aa
Merge branch 'main' into enh/check-plugin-manager-on-close
dalthviz Sep 2, 2025
61882dc
Connect task_status_manager with create_worker logic. Update threadin…
dalthviz Sep 3, 2025
25d1328
Merge branch 'main' into enh/check-plugin-manager-on-close
dalthviz Sep 4, 2025
9ad10f6
Merge branch 'main' into enh/check-plugin-manager-on-close
dalthviz Sep 5, 2025
f508136
Fix test_examples (handle confirm close dialog when workers are still…
dalthviz Sep 8, 2025
33e3e95
Merge branch 'enh/check-plugin-manager-on-close' of https://github.co…
dalthviz Sep 8, 2025
617474b
Apply suggestions from code review
dalthviz Sep 10, 2025
8f55526
Remove test_examples monkeypatch and patch task status manager over s…
dalthviz Sep 10, 2025
fb8473e
Merge branch 'main' into enh/check-plugin-manager-on-close
dalthviz Sep 15, 2025
f191660
Merge branch 'main' into enh/check-plugin-manager-on-close
dalthviz Sep 22, 2025
b76e8e8
Merge branch 'main' into enh/check-plugin-manager-on-close
dalthviz Sep 23, 2025
ea7d4e8
Revert changes to examples. Make task status manager per window and u…
dalthviz Sep 23, 2025
025e50a
Some code cleanup
dalthviz Sep 23, 2025
77686ee
Testing
dalthviz Sep 24, 2025
7f61ada
Revert track_status kwarg for worker creation. Allow programatic clos…
dalthviz Sep 24, 2025
fbaa5df
Merge branch 'main' into enh/check-plugin-manager-on-close
brisvag Sep 26, 2025
e4eea2e
Merge branch 'main' into enh/check-plugin-manager-on-close
brisvag Sep 26, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/napari/_qt/_qapp_model/qactions/_plugins.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ def _show_plugin_install_dialog(window: Window) -> None:
# This callback is only used when this package is available, thus we do not check
from napari_plugin_manager.qt_plugin_dialog import QtPluginDialog

QtPluginDialog(window._qt_window).exec_()
window._qt_window._plugin_manager_dialog = QtPluginDialog(
window._qt_window
)
if window._qt_window._plugin_manager_dialog is not None:
window._qt_window._plugin_manager_dialog.exec_()


def _show_plugin_err_reporter(window: Window) -> None:
Expand Down
19 changes: 15 additions & 4 deletions src/napari/_qt/dialogs/confirm_close_dialog.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,33 +14,44 @@


class ConfirmCloseDialog(QDialog):
def __init__(self, parent, close_app=False) -> None:
def __init__(
self,
parent,
close_app=False,
display_checkbox=True,
extra_info='',
) -> None:
super().__init__(parent)
extra_info = f'\n\n{extra_info}' if extra_info else ''
cancel_btn = QPushButton(trans._('Cancel'))
close_btn = QPushButton(trans._('Close'))
close_btn.setObjectName('warning_icon_btn')
icon_label = QWidget()

self.do_not_ask = QCheckBox(trans._('Do not ask in future'))
self.do_not_ask.setVisible(display_checkbox)
self._display_checkbox = display_checkbox

if close_app:
self.setWindowTitle(trans._('Close Application?'))
text = trans._(
"Do you want to close the application? ('{shortcut}' to confirm). This will close all Qt Windows in this process",
"Do you want to close the application? ('{shortcut}' to confirm). This will close all Qt Windows in this process{extra_info}",
shortcut=QKeySequence('Ctrl+Q').toString(
QKeySequence.NativeText
),
extra_info=extra_info,
)
close_btn.setObjectName('error_icon_btn')
close_btn.setShortcut(QKeySequence('Ctrl+Q'))
icon_label.setObjectName('error_icon_element')
else:
self.setWindowTitle(trans._('Close Window?'))
text = trans._(
"Confirm to close window (or press '{shortcut}')",
"Confirm to close window (or press '{shortcut}'){extra_info}",
shortcut=QKeySequence('Ctrl+W').toString(
QKeySequence.NativeText
),
extra_info=extra_info,
)
close_btn.setObjectName('warning_icon_btn')
close_btn.setShortcut(QKeySequence('Ctrl+W'))
Expand Down Expand Up @@ -70,6 +81,6 @@ def __init__(self, parent, close_app=False) -> None:
self.cancel_btn = cancel_btn

def accept(self):
if self.do_not_ask.isChecked():
if self._display_checkbox and self.do_not_ask.isChecked():
get_settings().application.confirm_close_window = False
super().accept()
22 changes: 20 additions & 2 deletions src/napari/_qt/qt_main_window.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@
running_as_constructor_app,
)
from napari.utils.notifications import Notification
from napari.utils.task_status import task_status_manager
from napari.utils.theme import _themes, get_system_theme
from napari.utils.translations import trans

Expand Down Expand Up @@ -135,6 +136,7 @@ def __init__(
super().__init__(parent)
self._ev = None
self._window = window
self._plugin_manager_dialog = None
self._qt_viewer = QtViewer(viewer, show_welcome_screen=True)
self._quit_app = False

Expand Down Expand Up @@ -490,8 +492,9 @@ def close(self, quit_app=False, confirm_need=False):
return super().close()
confirm_need_local = confirm_need and self._is_close_dialog[quit_app]
self._is_close_dialog[quit_app] = False

# here we save information that we could request confirmation on close
# So fi function `close` is called again, we don't ask again but just close
# So if function `close` is called again, we don't ask again but just close
if (
not confirm_need_local
or not get_settings().application.confirm_close_window
Expand Down Expand Up @@ -602,15 +605,29 @@ def closeEvent(self, event):

Regardless of whether cmd Q, cmd W, or the close button is used...
"""
task_status = task_status_manager.get_status()
if (
task_status
and ConfirmCloseDialog(
self,
close_app=False,
extra_info='\n'.join(task_status),
display_checkbox=False,
).exec_()
!= QDialog.Accepted
) or (
Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirming that this means the tasks busy dialog box will always be shown regardless of the user's preference to show the warning on a regular close

Copy link
Member Author

@dalthviz dalthviz Aug 26, 2025

Choose a reason for hiding this comment

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

Yep, as long as there are tasks with a PENDING or BUSY status the dialog asking to confirm the application close (with a description of the tasks being processed) will be shown

Copy link
Collaborator

Choose a reason for hiding this comment

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

And it means that it is shown even if action is connected to another napari window?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the napari windows are in the same process I think they could end up sharing the task manager instance indeed 🤔 It is possible to have multiple napari windows launched from the same process? Do you have a code snippet I could use to check this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: the multiple_viewers_.py is an example of this possibility

event.spontaneous()
and get_settings().application.confirm_close_window
and self._qt_viewer.viewer.layers
and ConfirmCloseDialog(self, False).exec_() != QDialog.Accepted
and ConfirmCloseDialog(self, close_app=False).exec_()
!= QDialog.Accepted
):
event.ignore()
return

if task_status_manager.is_busy():
task_status_manager.cancel_all()

self.status_thread.close_terminate()
self.status_thread.wait()

Expand Down Expand Up @@ -704,6 +721,7 @@ def __init__(self, viewer: 'Viewer', *, show: bool = True) -> None:

# Connect the Viewer and create the Main Window
self._qt_window = _QtMainWindow(viewer, self)

qapp.installEventFilter(self._qt_window)

# connect theme events before collecting plugin-provided themes
Expand Down
46 changes: 46 additions & 0 deletions src/napari/utils/_tests/test_task_status.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
from unittest.mock import Mock

from napari.utils.task_status import (
Status,
register_task_status,
task_status_manager,
update_task_status,
)


def test_task_status():
"""test task status registration and update using the utils.task_status module."""
# Check task status registration
cancel_callback_mock = Mock()
task_status_id = register_task_status(
'test-task-status',
Status.BUSY,
'Register task status busy',
cancel_callback=cancel_callback_mock,
)
assert task_status_manager.is_busy()
assert task_status_manager.get_status() == [
'test-task-status: Register task status busy'
]

# Check task status update
update_task_status(
task_status_id, Status.DONE, description='Register task status done'
)
assert not task_status_manager.is_busy()
assert task_status_manager.get_status() == []
update_task_status(
task_status_id,
Status.PENDING,
description='Register task status pending',
)
assert task_status_manager.is_busy()
assert task_status_manager.get_status() == [
'test-task-status: Register task status pending'
]

# Check cancel behavior
task_status_manager.cancel_all()
cancel_callback_mock.assert_called_once()
assert task_status_manager.get_status() == []
assert not task_status_manager.is_busy()
152 changes: 152 additions & 0 deletions src/napari/utils/task_status.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
import datetime
import uuid
from enum import auto
from typing import Optional

from napari.utils.misc import Callable, StringEnum


class Status(StringEnum):
PENDING = auto()
BUSY = auto()
DONE = auto()
CANCELLED = auto()
FAILED = auto()
Comment on lines 9 to 14
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, over napari-plugin-manager a sort of fallback of this enum is in place. Over the comment there, I left this comment regarding the match between different scenarios and the statuses: napari/napari-plugin-manager#174 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: This enum should be updated following the discussion at napari/napari-plugin-manager#174



class TaskStatusItem:
def __init__(
self,
provider: str,
status: Status,
description: str,
cancel_callback: Optional[Callable] = None,
) -> None:
self.id: uuid.UUID = uuid.uuid4()
self._provider = provider
self._timestamp = [self._timestap()]
self._status = [status]
self._description = [description]
self._cancel_callback = cancel_callback

def _timestap(self) -> str:
return datetime.datetime.now().isoformat()

def __str__(self) -> str:
return f'TaskStatusItem: ({self._provider}, {self.id}, {self._timestamp[-1]}, {self._status[-1]}, {self._description[-1]})'

def update(self, status: Status, description: str) -> None:
self._timestamp.append(self._timestap())
self._status.append(status)
self._description.append(description)

def cancel(self) -> bool:
self.update(Status.CANCELLED, '')
if self._cancel_callback is not None:
return self._cancel_callback()
return False

def state(self) -> tuple[str, str, Status, str]:
return (
self._provider,
self._timestamp[-1],
self._status[-1],
self._description[-1],
)


class TaskStatusManager:
"""
A task status manager, to store status of long running processes/tasks.

Only one instance is in general available through napari.

napari methods and plugins can use it to register and update
long running tasks.
"""

_tasks: dict[uuid.UUID, TaskStatusItem]

def __init__(self) -> None:
# Note: we are using a dict here that may not be thread-safe; however
# given the that the values from it are added/updated using an UUID
# collision chances are low and it should be ok as long as operations
# that require its iteration (`is_busy`, `get_status`, `cancel_all`)
# are done when no task status additions are scheduled (i.e when
# closing the application).
self._tasks: dict[uuid.UUID, TaskStatusItem] = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Any concerns about concurrent access here? Would it be better to have a Queue of small dataclasses that bundle the uuid and the status?

Copy link
Member Author

Choose a reason for hiding this comment

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

For the moment not much since to add and update an item uuids are being used and I think the only point where this dict gets iterated is when calling is_busy, get_status and cancel_all (logic that currently only gets triggered when wanting to close the whole app which I think is a point where not other task can get added? 🤔). However, if using a queue to handle the addition of tasks still makes sense let me know!

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. I'll let others chime in in case they share these concerns, but for now a comment would suffice. Something along the lines "note we are using a dict here that may not be thread-safe; however given the XXX conditions, it should be ok as long as we only perform XXXX tasks in XXX endpoints".


def register_task_status(
self,
provider: str,
task_status: Status,
description: str,
cancel_callback: Optional[Callable] = None,
) -> uuid.UUID:
item = TaskStatusItem(
provider, task_status, description, cancel_callback
)
self._tasks[item.id] = item
return item.id

def update_task_status(
self,
status_id: uuid.UUID,
task_status: Status,
description: str = '',
) -> bool:
if status_id in self._tasks:
item = self._tasks[status_id]
item.update(task_status, description)
return True

return False

def is_busy(self) -> bool:
for _, item in self._tasks.items():
if item.state()[2] in [Status.PENDING, Status.BUSY]:
return True
return False

def get_status(self) -> list[str]:
messages = []
for _, item in self._tasks.items():
provider, ts, status, description = item.state()
if status in [Status.PENDING, Status.BUSY]:
messages.append(f'{provider}: {description}')

return messages

def cancel_all(self) -> None:
for _, item in self._tasks.items():
item.cancel()


task_status_manager = TaskStatusManager()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not have a feeling like introducing this as a part of the public API with a global variable does not feel like a proper solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

So if I'm understanding correctly, having this as module constant doesn't feel appropiate, right? Maybe is better to instanciate a task manager per window? That makes sense to me (and more if it is possible to launch multiple napari windows at the same time from the same process) but let me know what you think!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. It is possible to launch multiple napari windows in one process. And when providing such a public API, it needs to be ready for a multi-window environment.

We may introduce some private API for the meantime if we want to iterate over to provide a solution.

There is no need to provide a public API for such a feature at the beginning. And even possible future API may be functional.



def register_task_status(
provider: str,
task_status: Status,
description: str,
cancel_callback: Optional[Callable] = None,
) -> uuid.UUID:
"""
Register a long running task.
"""
return task_status_manager.register_task_status(
provider, task_status, description, cancel_callback
)


def update_task_status(
task_status_id: uuid.UUID,
status: Status,
description: str = '',
) -> bool:
"""
Update a long running task.
"""
return task_status_manager.update_task_status(
task_status_id, status, description
)
Loading