Skip to content

Remove the plot customization dialog #14034

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.

Sign up for GitHub

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

Merged
MagnusSletten merged 5 commits into from
Jul 29, 2026
Merged

Remove the plot customization dialog #14034

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0676776
Move color chooser into plot controls
MagnusSletten Jul 27, 2026
bbccedb
Move handling of titles/axis from plotcustomizer
MagnusSletten Jul 29, 2026
760d9bb
Remove the plot customization toolbar button
MagnusSletten Jul 29, 2026
a1bce91
Delete the unused plot customization dialog
MagnusSletten Jul 28, 2026
69bf6af
Delete unused plot config history and copy dialog
MagnusSletten Jul 28, 2026
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
53 changes: 23 additions & 30 deletions src/ert/gui/plotting/plot_window.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,10 @@
from ert.services import ServerBootFail
from ert.utils import log_duration

from .customization_dialog import PlotCustomizer
from .plot_api import EnsembleObject, PlotApi, PlotApiKeyDefinition
from .utils import PlotConfig, PlotContext
from .utils import PlotConfigFactory, PlotContext
from .utils.observation_locations import transform_observation_locations
from .utils.plot_color_palettes import TABLEAU_10_COLOR_CYCLE
from .utils.plot_types import ObservationPlotLocations
from .utils.qt_creator import create_group_box, create_group_layout, create_side_panel
from .widgets.data_type_keys_widget import DataTypeKeysWidget
Expand Down Expand Up @@ -206,8 +206,9 @@ def __init__(
self._key_definitions = []
QApplication.restoreOverrideCursor()

self._plot_customizer = PlotCustomizer(self, self._key_definitions)
self._plot_customizer.settingsChanged.connect(self.keySelected)
self._titles: dict[str, str] = {}
self._x_labels: dict[str, str | None] = {}

@eilskra eilskra Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Iirc the previous default was "Unnamed". Unsure if this is something we should keep or change. Thoughts?

@MagnusSletten MagnusSletten Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The default is "Unnamed" from the finalize_plots method itself but it's never actually used. All the finalize_plots calls use custom ones passed as parameters, e.g:

 default_x_label = "Date" if plot_context.is_date_support_active() else "Index"
        ...
        PlotTools.finalizePlot(
            plot_context,
            figure,
            axes,
            default_x_label=default_x_label,
            default_y_label="Value",
        )

So there's currently no change in behaviour when the labels are None.

self._y_labels: dict[str, str | None] = {}
self._central_tab = QTabWidget()

central_widget = QWidget()
Expand Down Expand Up @@ -266,7 +267,7 @@ def __init__(

self._ensemble_selection_widget = EnsembleSelectionWidget(
plot_case_objects,
self._plot_customizer.get_plot_config().get_number_of_colors(),
len(TABLEAU_10_COLOR_CYCLE),
)

self._ensemble_selection_widget.ensembleSelectionChanged.connect(
Expand Down Expand Up @@ -543,9 +544,10 @@ def fetch_data(
except BaseException as e:
handle_exception(e)

plot_config = PlotConfig.create_copy(
self._plot_customizer.get_plot_config()
)
plot_config = PlotConfigFactory.create_plot_config_for_key(key_def)
plot_config.set_title(self._titles.get(key_def.key, key_def.key))
plot_config.set_x_label(self._x_labels.get(key_def.key))
plot_config.set_y_label(self._y_labels.get(key_def.key))
plot_config.set_legend_enabled(self._general_options.legend_checkbox_state)
plot_config.set_grid_enabled(self._general_options.grid_checkbox_state)
plot_config.set_line_color_cycle(self._general_options.get_color_cycle())
Expand Down Expand Up @@ -656,7 +658,6 @@ def add_plot_widget(
enabled: bool = True,
) -> None:
plot_widget = PlotWidget(name, plotter)
plot_widget.customizationTriggered.connect(self.toggle_customize_dialog)
plot_widget.axisLabelEditRequested.connect(self._edit_axis_label)
plot_widget.titleEditRequested.connect(self._edit_title)
plot_widget.layer_index_changed.connect(self.layer_index_changed)
Expand All @@ -669,11 +670,14 @@ def _edit_axis_label(self, axis: str) -> None:
label_names = {"x": "x-label", "y": "y-label"}
if axis not in label_names:
raise ValueError(f"Unknown axis '{axis}'. Expected 'x' or 'y'.")
key_def = self.getSelectedKey()
if key_def is None:
return
Comment on lines +673 to +675

@eilskra eilskra Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can key_def ever be None?

@MagnusSletten MagnusSletten Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(method) def getSelectedKey(self: Self@PlotWindow) -> (PlotApiKeyDefinition | None)

At least according to the declared typing. I can check this behaviour though. The none-checks was also there prior to this PR.

label_name = label_names[axis]
title = f"Edit {label_name}"
prompt = f"New {label_name}:"
plot_config = self._plot_customizer.get_plot_config()
current_label = plot_config.x_label() if axis == "x" else plot_config.y_label()
labels = self._x_labels if axis == "x" else self._y_labels
current_label = labels.get(key_def.key)
if current_label is None:
current_widget = self._central_tab.currentWidget()
if isinstance(current_widget, PlotWidget) and current_widget._figure.axes:
Expand All @@ -689,38 +693,30 @@ def _edit_axis_label(self, axis: str) -> None:
if not accepted:
return
new_label: str | None = new_label_text or None
if axis == "x":
plot_config.set_x_label(new_label)
else:
plot_config.set_y_label(new_label)
self._plot_customizer.update_plot_config(plot_config)
labels[key_def.key] = new_label

@eilskra eilskra Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we set it as None to trigger the default name?

@MagnusSletten MagnusSletten Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep!

Essentially because of this code in plot_tools.py:

if config.x_label() is None:
    config.set_x_label(default_x_label)

self.update_plot()

def _edit_title(self) -> None:
key_def = self.getSelectedKey()

@eilskra eilskra Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar question as on R673-675, if needed, should be extracted since repeated code

@MagnusSletten MagnusSletten Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Early return here cannot easily be extracted. The extraction would also take more lines of code than having the repeats of None-checks here. It can be shorter though, e.g

if (key_def := self.getSelectedKey()) is None:
    return

if key_def is None:
return
title = "Edit title"
plot_config = self._plot_customizer.get_plot_config()
new_title, accepted = self._general_options.get_text_input(
title,
"New title:",
plot_config.title(),
self._titles.get(key_def.key, key_def.key),
)
if not accepted:
return
if new_title:
plot_config.set_title(new_title)
else:
key_def = self.getSelectedKey()
if key_def is None:
return
plot_config.set_title(key_def.key)
self._plot_customizer.update_plot_config(plot_config)
self._titles[key_def.key] = new_title or key_def.key
self.update_plot()

@showWaitCursorWhileWaiting
def keySelected(self) -> None:
key_def = self.getSelectedKey()
if key_def is None:
self._show_no_data_message()
return
self._plot_customizer.switch_plot_config_history(key_def)

is_everest_specific_widget = key_def.metadata.get("data_origin") in {
"everest_objectives",
Expand Down Expand Up @@ -835,9 +831,6 @@ def everest_available_widget_selection(
self._prev_key_origin = key_def.metadata.get("data_origin")
self.update_plot()

def toggle_customize_dialog(self) -> None:
self._plot_customizer.toggle_customization_dialog()

def add_plot_widgets_from_plot_map(
self, plot_map: dict[str, Callable[[], Plotter]]
) -> None:
Expand Down
0 ...ing/customization_dialog/color_chooser.py → ...ng/widgets/plot_controls/color_chooser.py
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion src/ert/gui/plotting/widgets/plot_controls/custom_palette_dialog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@
QWidget,
)

from ert.gui.plotting.customization_dialog.color_chooser import ColorBox
from ert.gui.plotting.utils.logging_utils import log_plot_option_usage_once
from ert.gui.plotting.utils.plot_color_palettes import MINIMUM_COLOR_CYCLE_LENGTH
from ert.gui.plotting.widgets.plot_controls.color_chooser import ColorBox

logger = logging.getLogger(__name__)
Comment on lines 13 to 17

@eilskra eilskra Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@MagnusSletten is this relevant still?

@MagnusSletten MagnusSletten Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nope, not making any changes to the colorbox usage at this point. It's probably from an earlier state of the PR where it was larger redesign.


Expand Down
2 changes: 1 addition & 1 deletion src/ert/gui/plotting/widgets/plot_controls/observation_color.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@

from PyQt6.QtWidgets import QCheckBox, QHBoxLayout, QLabel, QWidget

from ert.gui.plotting.customization_dialog.color_chooser import ColorBox
from ert.gui.plotting.utils.logging_utils import log_plot_option_usage_once
from ert.gui.plotting.utils.plot_config import PlotConfig
from ert.gui.plotting.widgets.plot_controls.color_chooser import ColorBox

logger = logging.getLogger(__name__)
Comment on lines 6 to 10

Expand Down
99 changes: 62 additions & 37 deletions tests/ert/unit_tests/gui/tools/plot/test_plot_window.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from PyQt6.QtWidgets import QApplication, QCheckBox, QLabel, QPushButton, QToolTip
from pytestqt.qtbot import QtBot

from ert.config.breakthrough_config import BreakthroughConfig
from ert.config.distribution import RawSettings
from ert.config.gen_kw_config import DataSource, GenKwConfig
from ert.gui.plotting.ert_plots.gaussian_kde import plotGaussianKDE
Expand Down Expand Up @@ -1039,6 +1040,9 @@ def _create_plot_window_for_text_edit(
monkeypatch.setattr("ert.gui.plotting.plot_window.PlotApi", mock_plot_api_cls)
plot_window = PlotWindow(config_file="", ens_path=Path(), parent=None)
qtbot.addWidget(plot_window)
plot_window.getSelectedKey = MagicMock(
return_value=MagicMock(key="some_key", dimensionality=1, metadata={})
)
return plot_window


Expand Down Expand Up @@ -1086,12 +1090,8 @@ def test_that_sidebar_axis_label_edit_uses_configured_or_visible_label(
plot_window = _create_plot_window_for_text_edit(qtbot, monkeypatch)
get_text_input = MagicMock(return_value=("", False))
plot_window._general_options.get_text_input = get_text_input
plot_config = plot_window._plot_customizer.get_plot_config()
if axis == "x":
plot_config.set_x_label(configured_label)
else:
plot_config.set_y_label(configured_label)
plot_window._plot_customizer.update_plot_config(plot_config)
labels = plot_window._x_labels if axis == "x" else plot_window._y_labels
labels["some_key"] = configured_label
current_widget = plot_window._central_tab.currentWidget()
assert isinstance(current_widget, PlotWidget)
if visible_label is not None:
Expand Down Expand Up @@ -1140,36 +1140,19 @@ def test_that_axis_label_edit_updates_or_preserves_persistent_config(
plot_window = _create_plot_window_for_text_edit(qtbot, monkeypatch)
get_text_input = MagicMock(return_value=(new_label, accepted))
plot_window._general_options.get_text_input = get_text_input
plot_config = plot_window._plot_customizer.get_plot_config()
if axis == "x":
plot_config.set_x_label(current_label)
else:
plot_config.set_y_label(current_label)
plot_window._plot_customizer.update_plot_config(plot_config)
plot_window.update_plot = MagicMock()
labels = plot_window._x_labels if axis == "x" else plot_window._y_labels
labels["some_key"] = current_label

plot_window._edit_axis_label(axis)

expected_label = (new_label or None) if accepted else current_label
persisted_config = plot_window._plot_customizer.get_plot_config()
assert (
persisted_config.x_label() if axis == "x" else persisted_config.y_label()
) == expected_label
assert labels["some_key"] == expected_label
get_text_input.assert_called_once_with(
f"Edit {axis}-label", f"New {axis}-label:", current_label
)


def _create_plot_window_for_title_edit(
qtbot: QtBot,
monkeypatch: pytest.MonkeyPatch,
) -> PlotWindow:
plot_window = _create_plot_window_for_text_edit(qtbot, monkeypatch)
plot_window.getSelectedKey = MagicMock(
return_value=MagicMock(key="some_key", dimensionality=1, metadata={})
)
return plot_window


@pytest.mark.parametrize(
("dialog_value", "expected_title", "accepted"),
[
Expand All @@ -1190,19 +1173,15 @@ def test_that_title_edit_updates_or_preserves_persistent_config(
expected_title: str,
accepted: bool,
) -> None:
plot_window = _create_plot_window_for_title_edit(qtbot, monkeypatch)
plot_window = _create_plot_window_for_text_edit(qtbot, monkeypatch)
get_text_input = MagicMock(return_value=(dialog_value, accepted))
plot_window._general_options.get_text_input = get_text_input
plot_window.update_plot = MagicMock()
plot_window._plot_customizer._emit_changed_signal = MagicMock()
plot_config = plot_window._plot_customizer.get_plot_config()
plot_config.set_title("Existing title")
plot_window._plot_customizer.update_plot_config(plot_config)
plot_window._titles["some_key"] = "Existing title"

plot_window._edit_title()

persisted_config = plot_window._plot_customizer.get_plot_config()
assert persisted_config.title() == expected_title
assert plot_window._titles["some_key"] == expected_title
get_text_input.assert_called_once_with("Edit title", "New title:", "Existing title")


Expand Down Expand Up @@ -1258,15 +1237,61 @@ def test_that_clearing_custom_title_restores_key_title_when_rendering(
plot_window._general_options.get_text_input = MagicMock(return_value=("", True))
plot_window.update_plot()

plot_config = plot_window._plot_customizer.get_plot_config()
plot_config.set_title("Custom title")
plot_window._plot_customizer.update_plot_config(plot_config)
plot_window._titles["some_key"] = "Custom title"

@eilskra eilskra Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we not using the plot_config to set title here? Why are we overwriting the dictionary?

@MagnusSletten MagnusSletten Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There's currently no reachable plot_config after the removal of plot_customizer. The dict is what persists titles/labels for each key. I think we can improve on plot_config handling in another PR.

plot_window._edit_title()

plot_widget = plot_window._central_tab.currentWidget()
assert plot_widget._figure.axes[0].get_title() == "some_key"


@pytest.mark.slow
def test_that_breakthrough_response_title_keeps_the_breakthrough_prefix(
qtbot: QtBot,
monkeypatch: pytest.MonkeyPatch,
) -> None:
mock_plot_api_cls = MagicMock(spec=PlotApi)
mock_plot_api = MagicMock(spec=PlotApi)
mock_plot_api_cls.return_value = mock_plot_api

storage_version = "0.0"
mock_plot_api.api_version = storage_version
mock_plot_api.parameters_api_key_defs = []
mock_plot_api.responses_api_key_defs = [
PlotApiKeyDefinition(
"BREAKTHROUGH:WWCT:OP1",
index_type=None,
metadata={"data_origin": "summary"},
observations=False,
dimensionality=2,
response=BreakthroughConfig(),
)
]
mock_plot_api.get_all_ensembles.return_value = [
EnsembleObject(
"ensemble",
"ensemble",
False,
"experiment",
"2026-01-01T00:00:00",
)
]
mock_plot_api.data_for_response.return_value = pd.DataFrame({0: [1.0, 2.0, 3.0]})
mock_plot_api.has_history_data.return_value = False

monkeypatch.setattr(
"ert.gui.plotting.plot_window.get_storage_api_version",
lambda: storage_version,
)
monkeypatch.setattr("ert.gui.plotting.plot_window.PlotApi", mock_plot_api_cls)

plot_window = PlotWindow(config_file="", ens_path=Path(), parent=None)
qtbot.addWidget(plot_window)
plot_window.update_plot()

plot_widget = cast(PlotWidget, plot_window._central_tab.currentWidget())
assert plot_widget._figure.axes[0].get_title() == "BREAKTHROUGH:WWCT:OP1"


def test_that_resetting_axis_label_restores_histogram_default_label(
qtbot: QtBot,
) -> None:
Expand Down