Closes #233: made the Edit Playlist dialog conform better to the HIG

setup.cfg

@@ -1,5 +1,5 @@
 [flake8]
-extend-ignore = E203, E402, E722, W503, ANN002, ANN003, ANN101, ANN102, ANN204
+extend-ignore = E203, E402, E722, E731, W503, ANN002, ANN003, ANN101, ANN102, ANN204
 exclude = .git,__pycache__,build,dist,flatpak,.venv
 max-line-length = 88
 suppress-none-returning = True

sublime/adapters/configure_server_form.py

@@ -157,7 +157,7 @@ class ConfigureServerForm(Gtk.Box):
             if cpd.default is not None:
                 config_store[key] = config_store.get(key, cpd.default)
 
-            label = Gtk.Label(cpd.description + ":", halign=Gtk.Align.END)
+            label = Gtk.Label(cpd.description, halign=Gtk.Align.END)
 
             input_el_box = Gtk.Box()
             self.entries[key] = cast(

sublime/adapters/filesystem/adapter.py

@@ -442,23 +442,22 @@ class FilesystemAdapter(CachingAdapter):
         with self.db_write_lock, models.database.atomic():
             self._do_ingest_new_data(data_key, param, data)
 
-    def invalidate_data(
+    def invalidate_data(self, key: CachingAdapter.CachedDataKey, param: Optional[str]):
-        self, function: CachingAdapter.CachedDataKey, param: Optional[str]
+        print("invalidate", key, param)
-    ):
         assert self.is_cache, "FilesystemAdapter is not in cache mode!"
 
         # Wrap the actual ingestion function in a database lock, and an atomic
         # transaction.
         with self.db_write_lock, models.database.atomic():
-            self._do_invalidate_data(function, param)
+            self._do_invalidate_data(key, param)
 
-    def delete_data(self, function: CachingAdapter.CachedDataKey, param: Optional[str]):
+    def delete_data(self, key: CachingAdapter.CachedDataKey, param: Optional[str]):
         assert self.is_cache, "FilesystemAdapter is not in cache mode!"
 
         # Wrap the actual ingestion function in a database lock, and an atomic
         # transaction.
         with self.db_write_lock, models.database.atomic():
-            self._do_delete_data(function, param)
+            self._do_delete_data(key, param)
 
     def _do_ingest_new_data(
         self,
@@ -479,9 +478,10 @@ class FilesystemAdapter(CachingAdapter):
             f"_do_ingest_new_data param={param} data_key={data_key} data={data}"
         )
 
+        # TODO refactor to deal with partial data.
         def setattrs(obj: Any, data: Dict[str, Any]):
             for k, v in data.items():
-                if v:
+                if v is not None:
                     setattr(obj, k, v)
 
         def ingest_directory_data(api_directory: API.Directory) -> models.Directory:
@@ -652,9 +652,9 @@ class FilesystemAdapter(CachingAdapter):
             return song
 
         def ingest_playlist(
-            api_playlist: Union[API.Playlist, API.Playlist]
+            api_playlist: Union[API.Playlist, API.Playlist], partial: bool = False
         ) -> models.Playlist:
-            playlist_data = {
+            playlist_data: Dict[str, Any] = {
                 "id": api_playlist.id,
                 "name": api_playlist.name,
                 "song_count": api_playlist.song_count,
@@ -664,10 +664,6 @@ class FilesystemAdapter(CachingAdapter):
                 "comment": getattr(api_playlist, "comment", None),
                 "owner": getattr(api_playlist, "owner", None),
                 "public": getattr(api_playlist, "public", None),
-                "_songs": [
-                    self._do_ingest_new_data(KEYS.SONG, s.id, s)
-                    for s in api_playlist.songs
-                ],
                 "_cover_art": self._do_ingest_new_data(
                     KEYS.COVER_ART_FILE, api_playlist.cover_art, None
                 )
@@ -675,6 +671,17 @@ class FilesystemAdapter(CachingAdapter):
                 else None,
             }
 
+            if not partial:
+                # If it's partial, then don't ingest the songs.
+                playlist_data.update(
+                    {
+                        "_songs": [
+                            self._do_ingest_new_data(KEYS.SONG, s.id, s)
+                            for s in api_playlist.songs
+                        ],
+                    }
+                )
+
             playlist, playlist_created = models.Playlist.get_or_create(
                 id=playlist_data["id"], defaults=playlist_data
             )
@@ -755,7 +762,7 @@ class FilesystemAdapter(CachingAdapter):
         elif data_key == KEYS.PLAYLISTS:
             self._playlists = None
             for p in data:
-                ingest_playlist(p)
+                ingest_playlist(p, partial=True)
             models.Playlist.delete().where(
                 models.Playlist.id.not_in([p.id for p in data])
             ).execute()
@@ -772,7 +779,7 @@ class FilesystemAdapter(CachingAdapter):
                 ingest_song_data(s)
 
             for p in data._playlists.values():
-                ingest_playlist(p)
+                ingest_playlist(p, partial=True)
 
         elif data_key == KEYS.SONG:
             return_val = ingest_song_data(data)

sublime/ui/common/__init__.py

@@ -1,5 +1,4 @@
 from .album_with_songs import AlbumWithSongs
-from .edit_form_dialog import EditFormDialog
 from .icon_button import IconButton, IconMenuButton, IconToggleButton
 from .load_error import LoadError
 from .song_list_column import SongListColumn
@@ -7,7 +6,6 @@ from .spinner_image import SpinnerImage
 
 __all__ = (
     "AlbumWithSongs",
-    "EditFormDialog",
     "IconButton",
     "IconMenuButton",
     "IconToggleButton",

sublime/ui/common/edit_form_dialog.py

@@ -1,160 +0,0 @@
-from typing import Any, List, Optional, Tuple
-
-from gi.repository import Gtk
-
-TextFieldDescription = Tuple[str, str, bool]
-BooleanFieldDescription = Tuple[str, str]
-NumericFieldDescription = Tuple[str, str, Tuple[int, int, int], int]
-OptionFieldDescription = Tuple[str, str, Tuple[str, ...]]
-
-
-# TODO (#233) get rid of this and just make a nice custom one for Playlists since I am
-# not using this anywhere else anymore.
-class EditFormDialog(Gtk.Dialog):
-    entity_name: str
-    title: str
-    initial_size: Tuple[int, int]
-    text_fields: List[TextFieldDescription] = []
-    boolean_fields: List[BooleanFieldDescription] = []
-    numeric_fields: List[NumericFieldDescription] = []
-    option_fields: List[OptionFieldDescription] = []
-    extra_label: Optional[str] = None
-    extra_buttons: List[Gtk.Button] = []
-
-    def get_object_name(self, obj: Any) -> str:
-        """
-        Gets the friendly object name. Can be overridden.
-        """
-        return obj.name if obj else ""
-
-    def get_default_object(self):
-        return None
-
-    def __init__(self, parent: Any, existing_object: Any = None):
-        editing = existing_object is not None
-        title = getattr(self, "title", None)
-        if not title:
-            if editing:
-                title = f"Edit {self.get_object_name(existing_object)}"
-            else:
-                title = f"Create New {self.entity_name}"
-
-        Gtk.Dialog.__init__(
-            self, title=title, transient_for=parent, flags=0,
-        )
-        if not existing_object:
-            existing_object = self.get_default_object()
-
-        self.set_default_size(*self.initial_size)
-
-        # Store a map of field label to GTK component.
-        self.data = {}
-
-        content_area = self.get_content_area()
-        content_grid = Gtk.Grid(
-            column_spacing=10, row_spacing=5, margin_left=10, margin_right=10,
-        )
-
-        # Add the text entries to the content area.
-        i = 0
-        for label, value_field_name, is_password in self.text_fields:
-            entry_label = Gtk.Label(label=label + ":")
-            entry_label.set_halign(Gtk.Align.START)
-            content_grid.attach(entry_label, 0, i, 1, 1)
-
-            entry = Gtk.Entry(
-                text=getattr(existing_object, value_field_name, ""), hexpand=True,
-            )
-            if is_password:
-                entry.set_visibility(False)
-            content_grid.attach(entry, 1, i, 1, 1)
-            self.data[value_field_name] = entry
-
-            i += 1
-
-        for label, value_field_name, options in self.option_fields:
-            entry_label = Gtk.Label(label=label + ":")
-            entry_label.set_halign(Gtk.Align.START)
-            content_grid.attach(entry_label, 0, i, 1, 1)
-
-            options_store = Gtk.ListStore(str)
-            for option in options:
-                options_store.append([option])
-
-            combo = Gtk.ComboBox.new_with_model(options_store)
-            combo.set_id_column(0)
-            renderer_text = Gtk.CellRendererText()
-            combo.pack_start(renderer_text, True)
-            combo.add_attribute(renderer_text, "text", 0)
-
-            field_value = getattr(existing_object, value_field_name)
-            if field_value:
-                combo.set_active(field_value.value)
-
-            content_grid.attach(combo, 1, i, 1, 1)
-            self.data[value_field_name] = combo
-
-            i += 1
-
-        # Add the boolean entries to the content area.
-        for label, value_field_name in self.boolean_fields:
-            entry_label = Gtk.Label(label=label + ":")
-            entry_label.set_halign(Gtk.Align.START)
-            content_grid.attach(entry_label, 0, i, 1, 1)
-
-            # Put the checkbox in the right box. Note we have to pad here
-            # since the checkboxes are smaller than the text fields.
-            checkbox = Gtk.CheckButton(
-                active=getattr(existing_object, value_field_name, False)
-            )
-            self.data[value_field_name] = checkbox
-            content_grid.attach(checkbox, 1, i, 1, 1)
-            i += 1
-
-        # Add the spin button entries to the content area.
-        for (
-            label,
-            value_field_name,
-            range_config,
-            default_value,
-        ) in self.numeric_fields:
-            entry_label = Gtk.Label(label=label + ":")
-            entry_label.set_halign(Gtk.Align.START)
-            content_grid.attach(entry_label, 0, i, 1, 1)
-
-            # Put the checkbox in the right box. Note we have to pad here
-            # since the checkboxes are smaller than the text fields.
-            spin_button = Gtk.SpinButton.new_with_range(*range_config)
-            spin_button.set_value(
-                getattr(existing_object, value_field_name, default_value)
-            )
-            self.data[value_field_name] = spin_button
-            content_grid.attach(spin_button, 1, i, 1, 1)
-            i += 1
-
-        if self.extra_label:
-            label_box = Gtk.Box(orientation=Gtk.Orientation.HORIZONTAL)
-            label_box.add(self.extra_label)
-            content_grid.attach(label_box, 0, i, 2, 1)
-            i += 1
-
-        button_box = Gtk.Box(orientation=Gtk.Orientation.HORIZONTAL)
-        for button, response_id in self.extra_buttons:
-            if response_id is None:
-                button_box.add(button)
-                button.set_margin_right(10)
-            else:
-                self.add_action_widget(button, response_id)
-
-        content_grid.attach(button_box, 0, i, 2, 1)
-
-        content_area.pack_start(content_grid, True, True, 10)
-
-        self.add_buttons(
-            Gtk.STOCK_CANCEL,
-            Gtk.ResponseType.CANCEL,
-            Gtk.STOCK_EDIT if editing else Gtk.STOCK_ADD,
-            Gtk.ResponseType.OK,
-        )
-
-        self.show_all()

sublime/ui/playlists.py

@@ -1,6 +1,6 @@
 from functools import lru_cache
 from random import randint
-from typing import Any, cast, Iterable, List, Tuple
+from typing import Any, cast, Dict, Iterable, List, Tuple
 
 from fuzzywuzzy import process
 from gi.repository import Gdk, Gio, GLib, GObject, Gtk, Pango
@@ -9,7 +9,6 @@ from sublime.adapters import AdapterManager, api_objects as API
 from sublime.config import AppConfiguration
 from sublime.ui import util
 from sublime.ui.common import (
-    EditFormDialog,
     IconButton,
     LoadError,
     SongListColumn,
@@ -17,16 +16,63 @@ from sublime.ui.common import (
 )
 
 
-class EditPlaylistDialog(EditFormDialog):
+class EditPlaylistDialog(Gtk.Dialog):
-    entity_name: str = "Playlist"
+    def __init__(self, parent: Any, playlist: API.Playlist):
-    initial_size = (350, 120)
+        Gtk.Dialog.__init__(self, transient_for=parent, flags=Gtk.DialogFlags.MODAL)
-    text_fields = [("Name", "name", False), ("Comment", "comment", False)]
-    boolean_fields = [("Public", "public")]
 
-    def __init__(self, *args, **kwargs):
+        # HEADER
-        delete_playlist = Gtk.Button(label="Delete Playlist")
+        self.header = Gtk.HeaderBar()
-        self.extra_buttons = [(delete_playlist, Gtk.ResponseType.NO)]
+        self._set_title(playlist.name)
-        super().__init__(*args, **kwargs)
+
+        cancel_button = Gtk.Button(label="Cancel")
+        cancel_button.connect("clicked", lambda _: self.close())
+        self.header.pack_start(cancel_button)
+
+        self.edit_button = Gtk.Button(label="Edit")
+        self.edit_button.get_style_context().add_class("suggested-action")
+        self.edit_button.connect(
+            "clicked", lambda *a: self.response(Gtk.ResponseType.APPLY)
+        )
+        self.header.pack_end(self.edit_button)
+
+        self.set_titlebar(self.header)
+
+        content_area = self.get_content_area()
+        content_grid = Gtk.Grid(column_spacing=10, row_spacing=10, margin=10)
+
+        make_label = lambda label_text: Gtk.Label(label_text, halign=Gtk.Align.END)
+
+        content_grid.attach(make_label("Playlist Name"), 0, 0, 1, 1)
+        self.name_entry = Gtk.Entry(text=playlist.name, hexpand=True)
+        self.name_entry.connect("changed", self._on_name_change)
+        content_grid.attach(self.name_entry, 1, 0, 1, 1)
+
+        content_grid.attach(make_label("Comment"), 0, 1, 1, 1)
+        self.comment_entry = Gtk.Entry(text=playlist.comment, hexpand=True)
+        content_grid.attach(self.comment_entry, 1, 1, 1, 1)
+
+        content_grid.attach(make_label("Public"), 0, 2, 1, 1)
+        self.public_switch = Gtk.Switch(active=playlist.public, halign=Gtk.Align.START)
+        content_grid.attach(self.public_switch, 1, 2, 1, 1)
+
+        content_area.add(content_grid)
+        self.show_all()
+
+    def _on_name_change(self, entry: Gtk.Entry):
+        text = entry.get_text()
+        if len(text) > 0:
+            self._set_title(text)
+        self.edit_button.set_sensitive(len(text) > 0)
+
+    def _set_title(self, playlist_name: str):
+        self.header.props.title = f"Edit {playlist_name}"
+
+    def get_data(self) -> Dict[str, Any]:
+        return {
+            "name": self.name_entry.get_text(),
+            "comment": self.comment_entry.get_text(),
+            "public": self.public_switch.get_active(),
+        }
 
 
 class PlaylistsPanel(Gtk.Paned):
@@ -673,53 +719,46 @@ class PlaylistDetailPanel(Gtk.Overlay):
 
         result = dialog.run()
         # Using ResponseType.NO as the delete event.
-        if result in (Gtk.ResponseType.OK, Gtk.ResponseType.NO):
+        if result not in (Gtk.ResponseType.APPLY, Gtk.ResponseType.NO):
-            if result == Gtk.ResponseType.OK:
+            dialog.destroy()
-                AdapterManager.update_playlist(
+            return
-                    self.playlist_id,
-                    name=dialog.data["name"].get_text(),
-                    comment=dialog.data["comment"].get_text(),
-                    public=dialog.data["public"].get_active(),
-                )
-            elif result == Gtk.ResponseType.NO:
-                # Delete the playlist.
-                confirm_dialog = Gtk.MessageDialog(
-                    transient_for=self.get_toplevel(),
-                    message_type=Gtk.MessageType.WARNING,
-                    buttons=Gtk.ButtonsType.NONE,
-                    text="Confirm deletion",
-                )
-                confirm_dialog.add_buttons(
-                    Gtk.STOCK_DELETE,
-                    Gtk.ResponseType.YES,
-                    Gtk.STOCK_CANCEL,
-                    Gtk.ResponseType.CANCEL,
-                )
-                confirm_dialog.format_secondary_markup(
-                    f'Are you sure you want to delete the "{playlist.name}" playlist?'
-                )
-                result = confirm_dialog.run()
-                confirm_dialog.destroy()
-                if result == Gtk.ResponseType.YES:
-                    AdapterManager.delete_playlist(self.playlist_id)
-                    playlist_deleted = True
-                else:
-                    # In this case, we don't want to do any invalidation of
-                    # anything.
-                    dialog.destroy()
-                    return
 
-            # Force a re-fresh of the view
+        if result == Gtk.ResponseType.APPLY:
-            self.emit(
+            AdapterManager.update_playlist(self.playlist_id, **dialog.get_data())
-                "refresh-window",
+        elif result == Gtk.ResponseType.NO:
-                {
+            # Delete the playlist.
-                    "selected_playlist_id": None
+            confirm_dialog = Gtk.MessageDialog(
-                    if playlist_deleted
+                transient_for=self.get_toplevel(),
-                    else self.playlist_id
+                message_type=Gtk.MessageType.WARNING,
-                },
+                buttons=Gtk.ButtonsType.NONE,
-                True,
+                text="Confirm deletion",
             )
+            confirm_dialog.add_buttons(
+                Gtk.STOCK_DELETE,
+                Gtk.ResponseType.YES,
+                Gtk.STOCK_CANCEL,
+                Gtk.ResponseType.CANCEL,
+            )
+            confirm_dialog.format_secondary_markup(
+                f'Are you sure you want to delete the "{playlist.name}" playlist?'
+            )
+            result = confirm_dialog.run()
+            confirm_dialog.destroy()
+            if result == Gtk.ResponseType.YES:
+                AdapterManager.delete_playlist(self.playlist_id)
+                playlist_deleted = True
+            else:
+                # In this case, we don't want to do any invalidation of
+                # anything.
+                dialog.destroy()
+                return
 
+        # Force a re-fresh of the view
+        self.emit(
+            "refresh-window",
+            {"selected_playlist_id": None if playlist_deleted else self.playlist_id},
+            True,
+        )
         dialog.destroy()
 
     def on_playlist_list_download_all_button_click(self, _):