Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Change CPThemes to use sets instead of bitmasks. Closes #2015. #2073

Merged
merged 24 commits into from

5 participants

@saikat

I've done a bit of profiling here by just checking both implementation on the load time for https://gomockingbird.com/mockingbird, and there doesn't seem to be a significant difference, though it does seem to have slowed down currentValueForThemeAttribute just a bit. With the bitmask implementation, these were the times on three loads of Mockingbird:

[CPView setThemeState]: 1.1, 3.2, 1.1
[CPView setValue:forThemeAttribute:inState]: 1.1ms, 6.4, 1.1
[CPView setValue:forThemeAttribute]: 21.7, 8.4, 12.2
[CPView currentValueForThemeAttribute]: 13.7, 13.8, 11.1

With the new implementation:
[CPView setThemeState]: 3.1, 1.1, 4.2
[CPView setValue:forThemeAttribute:inState]: 4.1, 2.1, 4.2
[CPView setValue:forThemeAttribute]: 13.4, 10.5, 14.6
[CPView currentValueForThemeAttribute]: 15.5, 14.7, 14.6

@daboe01

+1

@saikat

I wanted to mention -- I'm not completely sold on having both CPThemeState (which is the app-level interface for creating and interacting with ThemeStates) and ThemeState (which is an immutable object representing a ThemeState). I do think it's nice to have a method that handles creation/grabbing from cache behind the scenes for you. Perhaps I should make CPThemeState be the only JS object and just have a JS helper function replace the current CPThemeState() function? Any thoughts on what to call the helper function in that case?

Also since it doesn't seem to show up in the comments of the issue yet, this PR refers to Issue #2015 .

@saikat

Also, one final thought.

If people are unhappy with the slight performance hit from changing from bitmasks to sets, one idea I had for keeping the bitmask performance is to simply store both a set of state names as well as a bitmask representation for each ThemeState internally and also keep track of how many total ThemeStates have been defined in an app. All the ThemeState/CPThemeState methods would simply use the bitmask operations if the total number of states in the app is below 31 (without hardcoding this of course -- we could simply detect the first time a state gets reused). Once the total number of states goes above 31, the internal methods switch to using the set-based methods.

@aljungberg
Owner

So I guess we never concluded the discussion in the original issue. Why do you need so many states and can't we tweak our existing states to fit your use case? It would be nice to have a simple theme state.

(Also, not a big thing, but mixing states with | and & is nice.)

@saikat
@aljungberg
Owner

Ok, right, I get where you're coming from. That seems valid to me. I guess from a user perspective, the limit of number of available theme states is a little arbitrary.

On the performance I don't think using a hybrid approach with bitmasks when possible will gain us enough performance for the added complexity. And looking at your numbers so far performance seems pretty unaffected using the set method. So better to keep it simple.

+AppKit
+#acknowledged
milestone=0.9.8

@saikat
@cappbot
Collaborator

Milestone: Someday. Vote: 1. Labels: #acknowledged, AppKit. What's next? A reviewer should examine this issue.

@saikat saikat Clean up the interface a bit to consolidate ThemeState methods onto o…
…ne object. Keep CPThemeState as just a helper function.
745ad7c
@saikat

I've changed the interface a bit. All the methods are now on the ThemeState object and CPThemeState is just a function to create ThemeStates. The only thing I think maybe should get changed is the name "ThemeState". Perhaps "CPThemeStateObject" or "CPInternalThemeState" or something with a CP in front of it. Any suggestions welcome.

@saikat

@aljungberg doesn't seem like the milestone tag got recognized by cappbot.

@aljungberg aljungberg modified the milestone: 0.9.8, Someday
@cappbot
Collaborator

Milestone: 0.9.8. Vote: 1. Labels: #acknowledged, AppKit. What's next? A reviewer should examine this issue.

@aljungberg
Owner

Hmm yeah looks like it tried but got a 402 "PRECONDITION FAILED" error from Github. Should probably add more retries to Cappbot. So many things to do, so little time.

assignee=aljungberg

@cappbot
Collaborator

Assignee: aljungberg. Milestone: 0.9.8. Vote: 1. Labels: #acknowledged, AppKit. What's next? A reviewer should examine this issue.

@aljungberg aljungberg was assigned by saikat
AppKit/CPButton.j
@@ -268,7 +268,7 @@ CPButtonImageOffset = 3.0;
break;
case CPOffState:
- [self unsetThemeState:CPThemeStateSelected | CPButtonStateMixed | CPThemeStateHighlighted];
+ [self unsetThemeState:CPThemeState(CPThemeStateSelected, CPButtonStateMixed, CPThemeStateHighlighted)];
@aljungberg Owner

How about [self unsetThemeStates:@[CPThemeStateSelected, CPButtonStateMixed, CPThemeStateHighlighted]]? Doesn't that feel more clear somehow?

@saikat
saikat added a note

Yeah I could go either way here. The one thing I like about having it be CPThemeState(x, y, z) is that this is explicit that the combination of x/y/z is itself a ThemeState. It makes it so the user is only ever dealing with one kind of object (a ThemeState). I know this is probably not a real issue, but this is the one criticism I have for [self unsetThemeStates:@[CPThemeStateSelected, CPButtonStateMixed, CPThemeStateHighlighted]] -- it implies to the user that the array (CPThemeStateSelected, CPButtonStateMixed, CPThemeStateHighlighted) can be treated like a ThemeState, but a ThemeState and an array are not, in actuality, interchangeable. But I don't feel strongly enough either way here and would be happy to change it if you think I should.

@aljungberg Owner

I don't know if the user would be confused by that - arrays of <something> are usually not interchangeable with <something>. To me it'd just feel like a method that can handle either a ThemeState or an array of them, not that the array is the same thing. Like a convenience method even if everything boils down to single or compound ThemeState objects behind the scenes.

And if we make it so then wherever it matters an array will be acceptable: set, unset and has -- we can make them all handle arrays optionally.

@saikat
saikat added a note

Ok I can buy that. I can add array handling optionally in these methods. One question though -- will Objective-J give me a warning for these methods if the type for the parameter is ThemeSet but an array is passed in at some point in the future? I recall something about strict parameter type checking being in the works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
AppKit/CPButtonBar.j
@@ -250,15 +250,15 @@
currentButtonOffset += width - 1;
}
- [button setValue:normalColor forThemeAttribute:@"bezel-color" inState:CPThemeStateNormal | CPThemeStateBordered];
- [button setValue:highlightedColor forThemeAttribute:@"bezel-color" inState:CPThemeStateHighlighted | CPThemeStateBordered];
- [button setValue:disabledColor forThemeAttribute:@"bezel-color" inState:CPThemeStateDisabled | CPThemeStateBordered];
+ [button setValue:normalColor forThemeAttribute:@"bezel-color" inState:CPThemeState(CPThemeStateNormal, CPThemeStateBordered)];
+ [button setValue:highlightedColor forThemeAttribute:@"bezel-color" inState:CPThemeState(CPThemeStateHighlighted, CPThemeStateBordered)];
+ [button setValue:disabledColor forThemeAttribute:@"bezel-color" inState:CPThemeState(CPThemeStateDisabled, CPThemeStateBordered)];
@aljungberg Owner

And here too, maybe an array of states? The CPThemeState constructor feels unclear somehow, like it could be an AND or an OR. Not sure why an array strikes me as more clear.

@saikat
saikat added a note

Yeah same thoughts here as above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
AppKit/CPSegmentedControl.j
((6 lines not shown))
else
- _themeStates[aSegment] |= CPThemeStateDisabled;
+ _themeStates[aSegment] = CPThemeState(_themeStates[aSegment], CPThemeStateDisabled);
@aljungberg Owner

No ThemeState.addThemeStates(_themeStates[aSegment], CPThemeStateDisabled) for consistency?

@saikat
saikat added a note

It felt redundant to have ThemeState.addThemeStates when the functionality is the same as CPThemeState(x, y).

@aljungberg Owner

Agreed, but sometimes clarity and "least surprise" beats simplicity.

@saikat
saikat added a note

That's fair. I can add that method in. I'm deciding between keeping the addThemeStates/subtractThemeStates syntax vs. the syntax you suggest below. I don't think both should exist though (that seems too confusing and as a user I'd wonder when to use one vs. the other).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
AppKit/CPTheme.j
((126 lines not shown))
+ return ThemeState._cacheThemeState(new ThemeState(newStates));
+}
+
+ThemeState._cacheThemeState = function(aState)
+{
+ // We do this caching so themeState equality works. Basically, doing CPThemeState('foo+bar') === CPThemeState('bar', 'foo') will return true.
+ var themeState = CPThemeStates[String(aState)];
+ if (themeState === undefined)
+ {
+ themeState = aState;
+ CPThemeStates[String(themeState)] = themeState;
+ }
+ return themeState;
+}
+
+var CPThemeStates = {};
@aljungberg Owner

Maybe declare the cache above the _cacheThemeState for clarity. I guess one other advantage, except for getting === comparisons, is that we intern all these states and save some memory (since some states will probably be created over and over independently).

@saikat
saikat added a note

Will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aljungberg
Owner

Just thought of another syntax that could be nice:

// aState + CPThemeStateDisabled   
aState.and(CPThemeStateDisabled)  

// a mask of states
CPThemeStateSelected.and(CPButtonStateMixed).and(CPThemeStateHighlighted) 

// aState - CPThemeStateHighlighted
aState.without(CPThemeStateHighlighted)
@saikat

I ended up going with your syntax suggestion and changed all the CPView methods to take arrays as well as ThemeStates. Thanks!

@aljungberg aljungberg referenced this pull request from a commit
@aljungberg aljungberg Fixed: missing var, minor tweaks.
Refs #2073.
e6acc3c
@aljungberg aljungberg merged commit b6b4015 into from
@aljungberg
Owner

Merged, thanks! Also, extra thanks for spotting all the special cases like setThemeState: being called using "perform selector" style messages.

+#fixed

@cappbot
Collaborator

Assignee: aljungberg. Milestone: 0.9.8. Vote: 1. Labels: #fixed, AppKit. What's next? This issue is considered successfully resolved.

@Dogild
Collaborator

Hi,

I just tried this PR and there is something a bit weird for me. In the method setThemeState: and unsetThemeState: we sometimes get an Array, an object ThemeState, or CPNull (is it a bug ?).

Solutions could be to add a method unsetThemeStates: and setThemeStates: or to avoid (example in CPButton) to send array.

Thanks for your work anyway!

@saikat
@saikat
@Dogild
Collaborator

There is actually an issue with the CPCheckButton, CPView doesn't like when it gets an array for the CPOffState. If you call three times the method unsetThemeState: with one themeState, everything works fine

@aljungberg
Owner

@Dogild yeah we decided to support both formats to avoid a clunky syntax when setting combination states. So instead of,

[thing setThemeState:CPThemeStateSelected.and(CPButtonStateMixed).and(CPThemeStateHighlighted)];

which is long and hard to read you can do:

[thing setThemeState:[CPThemeStateSelected, CPButtonStateMixed, CPThemeStateHighlighted]]

The drawback is that, like you noticed, if you subclass you have to convert the array to a ThemeState first thing.

If we wanted a specific array setter method we could do that but I think the Objective-J-y name would be something more like setThemeStateWithArray:. I don't think adding an S would be very clear since it's not really a matter of plural vs singular. You could use an array to set a single state, or a ThemeState to set a combination.

[thing setThemeState:[CPThemeStateSelected]]
[thing setThemeState:CPThemeStateSelected.and(CPButtonStateMixed)];

How can the CPCheckButton bug be reproduced?

@Dogild
Collaborator

You can try in Tests/Manual/CPButtonTest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 21, 2014
  1. @saikat

    First round of changes to change theming system to use arrays and not…

    saikat authored
    … bitmasks as their underlying datatype
  2. @saikat

    Get jaking working

    saikat authored
  3. @saikat
  4. @saikat
  5. @saikat
  6. @saikat

    Fix accidental semicolon

    saikat authored
  7. @saikat
  8. @saikat

    Fix subtracting theme states

    saikat authored
  9. @saikat

    Get all tests to pass

    saikat authored
  10. @saikat
  11. @saikat
  12. @saikat
  13. @saikat
  14. @saikat
  15. @saikat

    Remove unused method in CPTheme

    saikat authored
  16. @saikat
  17. @saikat

    Add new unit tests for CPView

    saikat authored
  18. @saikat
  19. @saikat
Commits on Feb 24, 2014
  1. @saikat
Commits on Feb 26, 2014
  1. @saikat

    Clean up the interface a bit to consolidate ThemeState methods onto o…

    saikat authored
    …ne object. Keep CPThemeState as just a helper function.
Commits on Feb 28, 2014
  1. @saikat
Commits on Mar 2, 2014
  1. @saikat
  2. @saikat
Something went wrong with that request. Please try again.