The solution needs to be smart about cancellation of the settings dialog. In this case, all settings are rolled-back to their state at the time of the dialog start, last apply, or last OK. This could potentially mean all, some, or none of the settings values change on cancel. How do we obtain the set that are actually rolling back?

Added SettingChange and SettingTask classes. Notifications are now delivered by SettingsChooser to the Tier containing the Perk managing the settings dialog. The state object, setting name, and new value are provided with the event.

Left to implement:

1) Ability for a Perk to send a setting change notification.
2) DefaultDialogPerk should send notification of all restored settings when the settings dialog is cancelled. This requires a return value from the AEState.restore() method call, namely the names/values that were restored that do not match the current settings.
3) DefaultPerk should send notification of speech rate when it changes by key press. DefaultDialogPerk should register for SettingChange events and tell the chooser to update appropriately when it occurs.

Completed #1 and #2.

Completing #3 is tricky. Two facts:

1) Setting change notifications from Perks are only delivered to other Perks in the same Tier. 
2) The reference to the singleton settings chooser is only held in one Tier by the Perk that created it.

This causes a problem when a setting change occurs in an arbitrary Tier and we need to get a reference to the chooser in another arbitrary Perk.

I think the solution here might be to introduce the concept of a singleton Perk. I thought of this idea a while ago while working on the DefaultDialogPerk. It's wasteful to have multiple instances of this Perk when the settings chooser Task creates a singleton dialog. What if we could specify a Perk as being a singleton that only created on instance, but would load across all Tiers?

I'm not sure what it would take to implement this idea. (Probably not much.) For now, I'm marking this bug incomplete to move on to higher priority work. I will open a new report about the singleton idea and make it a blocker.

The settings chooser reference can be stored in a PerkState object for the DefaultDialogPerk. The state is shared across all Tiers.

Created attachment 77047 [details] [review]
Proposed fix

This patch is still a bit hacky, but it might be getting a few things right:

* Added a flag to Task.Tools.System.signalSetting that tells it to execute the event in all tiers. I might have missed an easier way of doing it. But for now it just iterates over the tiers in the tier manager. This allows DefaultDialogPerk, in whatever tier it may be to catch the event, and forward it to it's chooser instance. Why should the SettingChange be specific to one tier in the first place?
* Added a dictionary in SettingsChooser that maps (state, setting name) -> (widget, setting). This allows updateSetting to easily retrieve the correct widget and change it's value.

And something I will regret tomorrow:
* AEState subclasses could now override a method called settingChanged. It will be called every time an AEState attribute with a capitalized name is changed. I did this because I thought it would be cool to have a signal emitted automatically, but then I noticed that AEState does not really have a reference to anything useful (say, a subclass of Task.Tools.All). So it would be ugly for the singleton PerkState to just grab a random Perk instance to emit these signals.

I'll try to refine this patch later.

Created attachment 77171 [details] [review]
Proposed patch

Yeah, touching the AEState patch was a really terrible idea. Here is a revised version.
I hand edited this patch. Hopefully it works.

> * Added a flag to Task.Tools.System.signalSetting that tells it to execute the
> event in all tiers. I might have missed an easier way of doing it. But for now
> it just iterates over the tiers in the tier manager. This allows
> DefaultDialogPerk, in whatever tier it may be to catch the event, and forward
> it to it's chooser instance. Why should the SettingChange be specific to one
> tier in the first place?

Do we really need to forward all setting changes through all Tiers? I was trying to avoid this solution because performance degrades with N=number of open applications and loaded Perks. Plus, I'm not sure I want PerkX to receive notifications and the state object for PerkY.

Only one DefaultDialogPerk instance needs to receive the notification. That instance can contact the chooser. The only change that needs to be made is that the chooser instance needs to be stored in the DefaultDialogPerkState object so it can be accessed by any instance of the Perk. This seems to be the cleanest solution unless we implement singleton Perks one day.

> * Added a dictionary in SettingsChooser that maps (state, setting name) ->
> (widget, setting). This allows updateSetting to easily retrieve the correct
> widget and change it's value. 

Do we need the setting instance to be stored in the dictionary? I thought the new value was returned in the SettingTask so we could pass it to updateSetting in the chooser?

Also, can you confirm the chooser, the widget factory, the widgets, and the dictionary are getting garbage collected properly? Seems like we could easily be creating cyclic references here now which the Python GC doesn't properly handle. A weakref.WeakValueDictionary or weakref.WeakKeyDictionary might help if it is indeed a problem.

> * AEState subclasses could now override a method called settingChanged. It will
> be called every time an AEState attribute with a capitalized name is changed. I
> did this because I thought it would be cool to have a signal emitted
> automatically, but then I noticed that AEState does not really have a reference
> to anything useful (say, a subclass of Task.Tools.All). So it would be ugly for
> the singleton PerkState to just grab a random Perk instance to emit these
> signals.

I thought about this too the first time. The problem is that sometimes Perks want to change the values temporarily and then switch them right back without notifying observers. For instance, the DefaultPerk now grabs a style object (which is an AEState subclass), switches the spelling format option to phonetic, speaks something, and then switches it back in order to implement phonetic spelling of words. I don't think we'd want the setting dialog "flashing" for this brief change.

Created attachment 77303 [details] [review]
Proposed fix

Fixed for latest CVS snapshot.
This patch also includes a proposal for Bug 376101

Let's think about the feasibility of replacing raw access to state object attributes with always using Setting objects as object descriptors. 

http://docs.python.org/ref/descriptor-invocation.html

What we want is some kind of proxy object that has the type, name, and description metadata, and supports the observer pattern for objects that need to be notified about changes.

The problem is how do we access the metadata or other properties of the Setting objects once they're descriptors? For instance, say Setting becomes a descriptor object. If we have a setting called Zoom on an AEState object, then we'd want to continue accessing its value like so:

print self.perk_state.Zoom

But how can we get the metadata from the Zoom object? We can't because Zoom is evaluated as soon as its referenced. Therefore, if the value of Zoom is 10, this code

print self.perk_state.Zoom.description

reduces to

print 10.description

Which is clearly not what we want. This is the problem I ran into the first time I wrote the AEState class and decided on the simpler route of keeping the setting objects separate from the AEState object attributes.

(In reply to comment #8)
> 
> Do we really need to forward all setting changes through all Tiers? I was
> trying to avoid this solution because performance degrades with N=number of
> open applications and loaded Perks. Plus, I'm not sure I want PerkX to receive
> notifications and the state object for PerkY.
> 
> Only one DefaultDialogPerk instance needs to receive the notification. That
> instance can contact the chooser. The only change that needs to be made is that
> the chooser instance needs to be stored in the DefaultDialogPerkState object so
> it can be accessed by any instance of the Perk. This seems to be the cleanest
> solution unless we implement singleton Perks one day.
> 

Yeah, the patch below does just that, wouldn't call it elegant, but it is cleaner than anything else.

> Do we need the setting instance to be stored in the dictionary? I thought the
> new value was returned in the SettingTask so we could pass it to updateSetting
> in the chooser?
>

We needed the setting instance to retrieve the dictionary or list for Enum and Choice setting types. I referenced the list/dict to the widget in the WidgetFactory, so there is no need for a setting reference any more.
 
> Also, can you confirm the chooser, the widget factory, the widgets, and the
> dictionary are getting garbage collected properly? Seems like we could easily
> be creating cyclic references here now which the Python GC doesn't properly
> handle. A weakref.WeakValueDictionary or weakref.WeakKeyDictionary might help
> if it is indeed a problem.
> 

I didn't check if it is getting freed, not sure if it's cyclic, but i am using WeakValueDictionary just in case for the widget objects. The only thing that might concern us is the state in the key tuple. But I don't see much damage here because State objects should last throughout LSR's lifetime, maybe I am wrong.

> 
> I thought about this too the first time. The problem is that sometimes Perks
> want to change the values temporarily and then switch them right back without
> notifying observers. For instance, the DefaultPerk now grabs a style object
> (which is an AEState subclass), switches the spelling format option to
> phonetic, speaks something, and then switches it back in order to implement
> phonetic spelling of words. I don't think we'd want the setting dialog
> "flashing" for this brief change.
> 

Like you said, if there is good model/view separation, this should be possible some day.

Created attachment 77318 [details] [review]
Proposed fix

Comment on attachment 77318 [details] [review]
Proposed fix

>+class DefaultDialogPerkState(Perk.PerkState):
>+  '''
>+  A state class used to store a singleton chooser across multiple perks.
>+  '''
>+  _chooser = None
>+  def setChooser(self, chooser):
>+    self._chooser = chooser
>+  def getChooser(self):
>+    return self._chooser


Getter and setter probably aren't needed here. Convention for accessing state attributes is just to request them directly. The state object is just a big struct for the most part.

 
>+      self.perk.perk_state.setChooser(chooser)

All references like this one can be simplified to self.perk_state.setChooser(chooser). All Tasks in a Perk have a direct reference to both the Perk (self.perk) and the Perk state (self.perk_state).

>+      widget.values_dict = setting.values
>+      widget.set_active(widget.values_dict.index(setting.value))

What exactly is this doing?

>+    # Map each widget to a state and name
>+    self.setting_contexts = weakref.WeakValueDictionary()

I think I'd like the dictionary holding widgets and the code for updating them:

>+    try:
>+      widget = self.setting_contexts[(state, name)]
>+    except KeyError:
>+      return
>+    if isinstance(widget, gtk.CheckButton):
>+      widget.set_active(value)
>+    elif isinstance(widget, gtk.FileChooserButton):
>+      widget.set_filename(value)
>+    elif isinstance(widget, gtk.Entry):
>+      widget.set_text(value)
>+    elif isinstance(widget, gtk.HScale):
>+      widget.set_value(value)
>+    elif isinstance(widget, gtk.SpinButton):
>+      widget.set_value(value)
>+    elif isinstance(widget, gtk.ComboBox):
>+      widget.set_active(widget.values_dict.index(value))
>+    elif isinstance(widget, gtk.ColorButton):
>+      widget.set_color(gtk.gdk.Color(red=value[0],
>+                                     green=value[1],
>+                                     blue=value[2]))
>+      if len(value) > 3:
>+        widget.set_alpha(value[3])

to be in the WidgetFactory class. The updateSetting method should be a dumb proxy to an update() method on the WidgetFactory which would contain all of the above logic and the dictionary storing widgets. This also means the factory can hash the widgets as it builds them without burdening the SettingChooser._populateSection method with that task.

(In reply to comment #13) 
> >+      widget.values_dict = setting.values
> >+      widget.set_active(widget.values_dict.index(setting.value))
> 
> What exactly is this doing?
> 

We need this dict/list when we want to update the value of a combobox without a reference to a Setting object.

Besides that, I incorporated all of your comments in the patch below.

Created attachment 77365 [details] [review]
Proposed fix

Thanks for the changes Eitan. I've applied your latest patch to CVS.