After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 737071 - Improve our state machine, try to enforce the invariants better and avoid races
Improve our state machine, try to enforce the invariants better and avoid races
Status: RESOLVED FIXED
Product: gnome-photos
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: GNOME photos maintainer(s)
GNOME photos maintainer(s)
Depends on: 737023
Blocks:
 
 
Reported: 2014-09-21 11:07 UTC by Debarshi Ray
Modified: 2015-10-13 13:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Merge PhotosCollectionManager into PhotosItemManager (66.83 KB, patch)
2014-09-21 11:27 UTC, Debarshi Ray
none Details | Review
Merge PhotosCollectionManager into PhotosItemManager (72.28 KB, patch)
2014-09-24 10:11 UTC, Debarshi Ray
committed Details | Review
item-manager: Handle failures better (2.16 KB, patch)
2014-09-26 13:04 UTC, Debarshi Ray
committed Details | Review
item-manager: Short-circuit the failure case (1.55 KB, patch)
2015-10-09 16:54 UTC, Debarshi Ray
committed Details | Review
Merge PhotosModeController into PhotosItemManager (55.41 KB, patch)
2015-10-09 16:55 UTC, Debarshi Ray
none Details | Review
Merge PhotosModeController into PhotosItemManager (68.86 KB, patch)
2015-10-12 14:50 UTC, Debarshi Ray
committed Details | Review
embed: Hide any empty search bars that might have been floating around (1.70 KB, patch)
2015-10-13 13:56 UTC, Debarshi Ray
committed Details | Review

Description Debarshi Ray 2014-09-21 11:07:04 UTC
Right now the global classes like ItemManager and CollectionManager don't do a very good job of enforcing the various invariants that should be asserted in the application. For example, if the current active item is a collection then it should be the same as the current active collection. Things like these are left to the rest of the code, and it is easy to make a mistake in keeping these two classes in sync.

I think we should consider merging them together, so that it becomes harder to violate these conditions.

Second, the state transitions are very racy. Eg., if a user clicks on a new collection, then the ItemManager and CollectionManager will be updated in two separate steps, and each step will emit an active-changed. In between these two signal emissions the application will be in an incoherent state.

Another example is this code to move back from the preview:

static void
photos_main_toolbar_back_button_clicked (PhotosMainToolbar *self)
{
  PhotosMainToolbarPrivate *priv = self->priv;

  photos_base_manager_set_active_object (priv->item_mngr, NULL);
  photos_mode_controller_go_back (priv->mode_cntrlr);
}

The first line emits ItemManager:active-change and the second emits ModeController:window-mode-changed. In between these two the application will have (active_item = NULL, mode = PREVIEW), and depending on how the rest of the code reacts to these signals, they need to handle these transitional states carefully.

The history here is that the code started as a clone of gnome-documents which used to be a single view application. The current split view designs have more sophisticated interactions, so a more declarative approach makes it simpler to introduce changes in the application. Currently it is very easy for an unrelated change to introduce a regression.
Comment 1 Debarshi Ray 2014-09-21 11:13:31 UTC
For some reason, my WIP patch to merge ItemManager and CollectionManager is triggering bug 737023
Comment 2 Debarshi Ray 2014-09-21 11:27:20 UTC
Created attachment 286730 [details] [review]
Merge PhotosCollectionManager into PhotosItemManager
Comment 3 Debarshi Ray 2014-09-24 10:11:56 UTC
Created attachment 286964 [details] [review]
Merge PhotosCollectionManager into PhotosItemManager

I forgot to delete photos-collection-manager.[ch]
Comment 4 Debarshi Ray 2014-09-24 10:13:12 UTC
Comment on attachment 286964 [details] [review]
Merge PhotosCollectionManager into PhotosItemManager

Both me and pranavk have been using it for a while. Can't spot any regressions.
Comment 5 Debarshi Ray 2014-09-26 13:04:40 UTC
Created attachment 287154 [details] [review]
item-manager: Handle failures better
Comment 6 Debarshi Ray 2015-10-09 16:54:35 UTC
Created attachment 312969 [details] [review]
item-manager: Short-circuit the failure case
Comment 7 Debarshi Ray 2015-10-09 16:55:03 UTC
Created attachment 312970 [details] [review]
Merge PhotosModeController into PhotosItemManager
Comment 8 Debarshi Ray 2015-10-12 14:50:14 UTC
Created attachment 313124 [details] [review]
Merge PhotosModeController into PhotosItemManager
Comment 9 Debarshi Ray 2015-10-13 11:35:02 UTC
Seems to work fine. Let's get this in early in the cycle so that we have enough time to shake out the bugs.

Pushed to master.
Comment 10 Debarshi Ray 2015-10-13 13:56:55 UTC
Created attachment 313189 [details] [review]
embed: Hide any empty search bars that might have been floating around