Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored preset parsing #307

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

FedorZaytsev
Copy link
Collaborator

In this PR I am refactored current preset parsing system to make it easier to use and scale.

Problems with current parsing

  1. It uses enums which is not very scalable. In current system we have a switch for 20 cases which represent types. Parameters for the presets are mixed up - all of them (29) lies in a single CodingKeys enum.
  2. It is too broad. There is no single place where all the parameters are read. Some params are read in the ItemType initializator, some in GeneralParameter and so on.
  3. We need 5 additional objects to parse:
  • ItemType, ActionType, LongActionType, GeneralParameter - initial parsing
  • BarItemDefinition - storage for items above
  1. Parameters parsing is separated from an actual object. This limits some of the future changes (import preset from BTT)

TL;DR: Current system is good enough, but probably we can do it more scalable?

Proposal

I propose to make all widgets to be based on a single base class CustomTouchBarItem which would satisfy Decodable interface and put all parameter parsing into init(from decoder: Decoder).

Pros

  • Parameter parsing and object creation are close to each other.
  • Simpler (in some sense, read below) creation of a new widgets. You only need to correctly implement your class and register it in one(!) place.

Cons

  • Parsing is now mixed up with business logic which is not good too

Implementation details

All classes are inherited from a base class CustomTouchBarItem which satisfy Decodable. Each widget inherited from this class override class var typeIdentifier to expose its type identifier (for example appleScriptTitledButton or brightnessDown). Class also need to override init(from: Decoder) and parse all custom params. To make this class be visible for the parser you need to add this class into BarItemDefinition.types in ItemParsing.swift. Here is example how it looks like:

class NetworkBarItem: CustomButtonTouchBarItem {    
    private let flip: Bool
    
    private enum CodingKeys: String, CodingKey {
        case flip
    }
    
    override class var typeIdentifier: String {
        return "network"
    }
    
    init(identifier: NSTouchBarItem.Identifier, flip: Bool = false) {
        self.flip = flip
        super.init(identifier: identifier, title: " ")
        startMonitoringProcess()
    }

    required init?(coder _: NSCoder) {
        fatalError("init(coder:) has not been implemented")
    }
    
    required init(from decoder: Decoder) throws {
        let container = try decoder.container(keyedBy: CodingKeys.self)
        self.flip = try container.decodeIfPresent(Bool.self, forKey: .flip) ?? false
        
        try super.init(from: decoder)
        startMonitoringProcess()
    }
....
}

Implementation limitations

  1. I made typeIdentifier to be class var because I wanted to access it before creating class (need in ItemParsing.swift). Because of this I cannot enforce that everyone has overrided it and if someone would forget about it it wouldn't be able to load his class even if it is registered.
  2. It is easy to create a class which is inherited from NSCustomTouchBarItem, but not so simple to inherit from some other types like NSSlider or NSPopoverTouchBarItem. However it can be done (BrightnessViewController, GroupBarItem)
  3. When creating new Widget you may forget to call super.init(from: decoder). In this case it wouldn't decode basing params like width or text. Again, no way to enforce it.

Testing

While implementing I have tested all supported widgets. No regression was found. I would provide test preset later

TODO

  1. README
  2. Fix tests. Right now they are completely broken and I have no idea how to fix them, because it is some problem with libraries and stuff.

Fedor Zaitsev and others added 6 commits February 28, 2020 02:10
Added explanation for missing parameters (background, title and image)
AppleScriptTouchBarItem now allow to specify any number of icons which can be changed from the script. You cannot change icon from touch event. To change icon, you need to return array from your script with 2 values - title and icn name. More info in readme
Rewrote presets parsing. Instead of parsing presets in a separate file using bunch of huge enums it is now parsed inside each object.

To implement a new class:
1. Derive your class from CustomTouchBarItem (for static) or CustomButtonTouchBarItem (for buttons)
2. Override class var typeIdentifier with your object identificator
3. Override init(from decoder: Decoder) and read all custom json params you need
4. Don't forget to call super.init(identifier: CustomTouchBarItem.createIdentifier(type)) in the init() function
5. Add your new class to BarItemDefinition.types in ItemParsing.swift

Good example is PomodoroBarItem

If you want to inherid from some other NS class (NSSlider or NSPopoverTouchBarItem or other) then look into GroupBarItem and BrightnessViewController
@FedorZaytsev
Copy link
Collaborator Author

I know that it is a big commit, but I hope that someone can take a glance at it. Ready to fix anything.

P.S. I will be happy if someone will help me with tests - it seems that MTMRTest doesn't have some libraries or something.

@Toxblh
Copy link
Owner

Toxblh commented Jul 26, 2020

tests is failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants