architecture icon indicating copy to clipboard operation
architecture copied to clipboard

Proposal: Settings API

Open mickael-menu opened this issue 4 years ago • 4 comments

See the full proposal

mickael-menu avatar Aug 20 '21 16:08 mickael-menu

I think for the most part this is good. But a few things:

  1. I'm still not 100% clear what the difference is between PresentationProperties and PresentationSettings. If that could be made clearer under the Terms section, that would be helpful. Maybe even a class diagram or something
  2. I haven't really looked at the Swift implementation, but is it using Key-Value Observing at all?
  3. I'm not sure about the name SwitchProperty. Using "Switch" feels more like a user interface term. Other suggestions:
    1. ToggleProperty
    2. BooleanProperty
    3. BinaryProperty

stevenzeck avatar Aug 24 '21 01:08 stevenzeck

@stevenzeck

  1. I'm still not 100% clear what the difference is between PresentationProperties and PresentationSettings. If that could be made clearer under the Terms section, that would be helpful. Maybe even a class diagram or something

I agree that it is a bit confusing and I'm not 100% happy about this part. But I'm not sure how to make it clearer in the Terms section, maybe it's a sign that this part is not properly designed or named.

Basically I separated two things:

  • Properties which are dynamic values representing the current presentation state of the Navigator. The navigator also provides metadata about each property (ranges, constraints, localized description, etc.).
  • Settings are a bunch of values that the app would like the Navigator to use when computing the Properties values. The Navigator still decides what is the actual value and might ignore some of the settings, so it's not a "hard-set".

To associate the properties and settings, I added the concept of a Property Key which is just a string identifier, e.g. fontSize or com.company.customKey.

  1. I haven't really looked at the Swift implementation, but is it using Key-Value Observing at all?

There's no implementation yet but KVO would be a good fit yes, I think. I was thinking of Combine but it's only available on iOS 13+ so not usable in Readium.

  1. I'm not sure about the name SwitchProperty. Using "Switch" feels more like a user interface term. Other suggestions:

    1. ToggleProperty
    2. BooleanProperty
    3. BinaryProperty

I like ToggleProperty as well.

mickael-menu avatar Sep 06 '21 10:09 mickael-menu

To avoid the confusion between Presentation Settings and Properties, maybe we could just say that a list of Property Keys is passed to Presentation Properties ?

This way there's no need to add another concept/term.

HadrienGardeur avatar Sep 06 '21 13:09 HadrienGardeur

How about renaming PresentationSettings into PresentationKeys, keeping the same capabilities? This way we remove the concept of "Presentation Settings" and we just get a map of Presentation Key -> Value wrapped in a PresentationKeys object for convenience (type-safe accessors).

mickael-menu avatar Sep 06 '21 14:09 mickael-menu