Refactor: unifying command handling and improving layer separation #286

Author: TheNinth7

source/main/data/communications/web-requests/command/BaseCommandRequest.mc

@@ -89,7 +89,13 @@ class BaseCommandRequest extends BaseRequest {
     // If not on Wifi connection or if in sync mode, the command will be sent immediately
     // If on Wifi connection, the command will be handed over to the sync delegate and
     // sync mode will be started.
-    public function sendCommand( cmd as String ) as Void {
+    public function sendCommand( cmd as Item.ItemState ) as Void {
+
+        // Non-string commands are converted to a string
+        if( ! ( cmd instanceof String ) ) {
+            cmd = cmd.toString();
+        }
+        
         // Note that isWifiConnected()=true indicates only that Wi-Fi is available,
         // but the app still needs to switch into sync mode to connect to it. If _syncMode
         // is true, then we are already in sync mode and can send the command.

source/main/user-interface/features/sitemap/control/picker/numeric/NumericPickerDelegate.mc

@@ -44,7 +44,7 @@ class NumericPickerDelegate extends BaseDynamicPickerDelegate {
                 throw new NonFatalUserInterfaceException( NonFatalUserInterfaceException.EX_INVALID_STATE_TYPE );
             }
             // Logger.debug "NumericPickerDelegate.onAccept: new state=" + newState.toString() );
-            _menuItem.updateState( newState );
+            _menuItem.sendCommand( newState );
         }
         ViewStack.popView( WatchUi.SLIDE_RIGHT );
         return true;
@@ -58,7 +58,7 @@ class NumericPickerDelegate extends BaseDynamicPickerDelegate {
         if( ! _menuItem.getSitemapNumeric().isReleaseOnly() ) {
             if( _menuItem.getSitemapNumeric().getNumericItem().getNumericState() != _previousState ) {
                 // Logger.debug "NumericPickerDelegate.onCancel: reverting to state=" + _previousState.toString() );
-                _menuItem.updateState( _previousState );
+                _menuItem.sendCommand( _previousState );
             } else {
                 // Logger.debug "NumericPickerDelegate.onCancel: state did not change" );
             }
@@ -85,7 +85,7 @@ class NumericPickerDelegate extends BaseDynamicPickerDelegate {
         }
         // Update only if releaseOnly is false
         if( ! _menuItem.getSitemapNumeric().isReleaseOnly() ) {
-            _menuItem.updateState( state );
+            _menuItem.sendCommand( state );
         }
     }     
 }

source/main/user-interface/features/sitemap/menu-items/base/BaseCommandMenuItem.mc

@@ -6,6 +6,13 @@ import Toybox.Time;
 class BaseCommandMenuItem extends BaseWidgetMenuItem {
 
     private var _commandRequest as BaseCommandRequest?;
+
+    // The last time an internal update was applied. Internal updates are applied
+    // by some menu items directly after a command is sent to immediately reflect
+    // the new state. Storing this timestamp is required to apply the post-command
+    // hold time configured in the app settings.
+    private var _lastLocalStateUpdate as Moment?;
+
     private var _pendingCommand as Item.ItemState?;
 
     // Constructor
@@ -44,19 +51,29 @@ class BaseCommandMenuItem extends BaseWidgetMenuItem {
         return _pendingCommand != null;
     }
 
+    // Returns true if the item is still within the configured post-command hold time
+    // since the last internal state update.
+    public function isInHoldTime() as Boolean {
+        var postCommandHoldTime = AppSettings.getPostCommandHoldTime();
+        if( _lastLocalStateUpdate != null && postCommandHoldTime.value() > 0 ) {
+            return Time.now().lessThan( _lastLocalStateUpdate.add( postCommandHoldTime ) );
+        } else {
+            return false;
+        }
+    }
+
     public function onCommandComplete() as Void {
         if( _pendingCommand != null ) {
             // Perform an internal update of the state in the SitemapSwitch
             getSitemapWidget().updateState( _pendingCommand as Item.ItemState );
             _pendingCommand = null;
 
-            // Notify the base class that an internal state update was performed.
-            // This triggers the post-command hold time for state updates,
+            // This is used to apply the post-command hold time for state updates,
             // if configured in the app settings.
-            notifyStateUpdatedLocally();
+            _lastLocalStateUpdate = Time.now();
 
             // Update state displayed by the menu item
-            onStateUpdatedLocally();
+            onStateChangedLocally();
 
             WatchUi.requestUpdate();
        }
@@ -73,22 +90,23 @@ class BaseCommandMenuItem extends BaseWidgetMenuItem {
         _pendingCommand = null;
     }
 
-    public function onStateUpdatedLocally() as Void {
+    public function onStateChangedLocally() as Void {
         setIcon( getSitemapWidget().getIcon() );
-        onStateUpdated();
+        onStateChanged();
     }
 
-    public function onStateUpdated() as Void {
+    public function onStateChanged() as Void {
     }
 
-    // Send the command via the command request
+    // Sends the command via the CommandRequest.
+    // This method allows a command to be sent even if another command
+    // is currently pending. In that case, the pending command is replaced
+    // and the underlying request cancels the previous one.
+    // Subclasses that want to prevent sending a new command while one
+    // is pending should call hasPendingCommand() before invoking
+    // sendCommand().
     public function sendCommand( command as Item.ItemState ) as Void {
-
-        if( ! ( command instanceof String ) ) {
-            throw new GeneralException( "BaseCommandMenuItem.sendCommand supports only String arguments." );
-        }
-
-        if( ! hasPendingCommand() && _commandRequest != null ) {
+        if( _commandRequest != null ) {
             _pendingCommand = command;
             _commandRequest.sendCommand( command );
         }

source/main/user-interface/features/sitemap/menu-items/base/BaseWidgetMenuItem.mc

@@ -30,31 +30,24 @@ typedef BaseWidgetMenuItemOptions as {
 
 class BaseWidgetMenuItem extends BaseSitemapMenuItem {
 
+    // Store whether the subclass indicated that an action icon should be shown
+    // This information is passed into the constructor as option, but needed 
+    // also during updates
+    private var _isActionable as Boolean;
+
     // The submenu representing the nested elements, if there are any
     private var _page as PageMenu?;
-    
+
+    // The current sitemap widget associated with this menu item
+    private var _sitemapWidget as SitemapWidget;
+
     // Reference to the parent menu is required so that submenus
     // can navigate back. Submenus may be created not only in the
     // constructor, but also dynamically in updateWidget(), so we
     // store the parent menu as a member variable and use a weak
     // reference to avoid memory leaks due to circular references.
     private var _weakParent as WeakReference;
 
-    // Store whether the subclass indicated that an action icon should be shown
-    // This information is passed into the constructor as option, but needed 
-    // also during updates
-    private var _isActionable as Boolean;
-
-    // The last time an internal update was applied. Internal updates are applied
-    // by some menu items directly after a command is sent to immediately reflect
-    // the new state. Storing this timestamp is required to apply the post-command
-    // hold time configured in the app settings.
-    private var lastLocalStateUpdate as Moment?;
-
-    // The sitemap widget is stored, so in updateWidget() we can check if the
-    // state has changed, and if yes call onStateUpdated()
-    private var _sitemapWidget as SitemapWidget;
-
     // Constructor
     protected function initialize( 
         options as BaseWidgetMenuItemOptions
@@ -81,50 +74,35 @@ class BaseWidgetMenuItem extends BaseSitemapMenuItem {
         );
     }
 
+
     // Returns the current sitemap widget
     protected function getSitemapWidget() as SitemapWidget {
         return _sitemapWidget;
     }
 
+
     // Returns true if the widget is linked to a page (sub menu)
     public function hasPage() as Boolean {
         return _page != null;
     }
 
-    // Returns true if the item is still within the configured post-command hold time
-    // since the last internal state update.
-    public function isInHoldTime() as Boolean {
-        var postCommandHoldTime = AppSettings.getPostCommandHoldTime();
-        if( lastLocalStateUpdate != null && postCommandHoldTime.value() > 0 ) {
-            return Time.now().lessThan( lastLocalStateUpdate.add( postCommandHoldTime ) );
-        } else {
-            return false;
-        }
-    }
 
     // Subclasses must override this method to determine whether the given
     // `SitemapWidget` instance is compatible with this menu item type.
     public static function isMyType( sitemapWidget as SitemapWidget ) as Boolean { 
         throw new AbstractMethodException( "BaseSitemapMenuItem.getItemType" );
     }
 
-    // Must be called by subclasses when they perform an internal state update.
-    // Some menu items use internal updates to immediately reflect the new state
-    // after a command is sent. This notification stores the time of the update,
-    // which is required to apply the configured post-command hold time.
-    protected function notifyStateUpdatedLocally() as Void {
-        lastLocalStateUpdate = Time.now();
-    }
 
     /*
-     * Subclasses need to override this method to process state updates.
-     * Note: updateWidget() can be overriden if other widget properties have an 
-     * effect on the 
+     * Subclasses can to override this method to process state changes.
+     * Note: this is triggered only when the state has changed. If updates
+     * to other widget data elements should be processed independent of a
+     * state change, then updateWidget() should be overriden to process those.
      */
-    public function onStateUpdated() as Void {
-        //throw new AbstractMethodException( "BaseWidgetMenuItem.onStateUpdated" );
-    }
-    
+    public function onStateChanged() as Void {}
+
+
     // Handles selection of the menu item.
     //
     // If a submenu is present, it is opened on selection. This typically takes precedence
@@ -195,10 +173,10 @@ class BaseWidgetMenuItem extends BaseSitemapMenuItem {
      * parts of the update, but they must also call the base class’s
      * updateWidget() to ensure core functionality is preserved.
      *
-     * To process state updates subclasses SHOULD NOT use this function, but
-     * instead override onStateUpdated(). This method is only called
+     * To process state changes subclasses SHOULD NOT use this function, but
+     * instead override onStateChanged(). This method is only called
      * if the state has changed, subclasses may use that implementation also
-     * to process local updates.
+     * to process local changes.
      */
     public function updateWidget( sitemapWidget as SitemapWidget ) as Void { 
         // Store the new widget instance
@@ -209,17 +187,19 @@ class BaseWidgetMenuItem extends BaseSitemapMenuItem {
         // during an update, we always use the async task queue
         processWidget( sitemapWidget, BasePageMenu.PROCESSING_ASYNC );
 
-        // Determine if the state as changed and if
-        // yes, call onStateUpdated()
+        // Determine if the display state or state as changed and if
+        // yes, call onStateChanged()
         var previousItem = previousSitemapWidget.getItem();
         var newItem = sitemapWidget.getItem();
         var hasStateChanged =
-            ( previousItem == null ) != ( newItem == null ) 
+            ! previousSitemapWidget.getDisplayState().equals( sitemapWidget.getDisplayState() )
+            || ( previousItem == null ) != ( newItem == null ) 
             || ( previousItem != null 
                  && newItem != null
                  && ! previousItem.getState().equals( newItem.getState() ) );
+
         if( hasStateChanged ) {
-            onStateUpdated();
+            onStateChanged();
         }
 
         WatchUi.requestUpdate();

source/main/user-interface/features/sitemap/menu-items/container/ContainerMenuItem.mc

@@ -30,12 +30,9 @@ class ContainerMenuItem extends BaseWidgetMenuItem {
         } );
     }
 
-    // Updates the menu item
-    public function updateWidget( sitemapWidget as SitemapWidget ) as Void {
-        BaseWidgetMenuItem.updateWidget( sitemapWidget );
-        if( ! ( sitemapWidget instanceof SitemapFrame || sitemapWidget instanceof SitemapGroup ) ) {
-            throw new GeneralException( "Sitemap element '" + sitemapWidget.getLabel() + "' was passed into ContainerMenuItem but is of a different type" );
-        }
-        setStateTextResponsive( sitemapWidget.getDisplayStateOrNull() );
+    // When the state changes, change the displayed state text
+    public function onStateChanged() as Void {
+        setStateTextResponsive( getSitemapWidget().getDisplayStateOrNull() );
     }
+
 }