...
[15:00:30 CST(-0600)] <lahabana> anastasiac: it doesn't seem that useful isn''t it
[15:01:58 CST(-0600)] <anastasiac> lahabana, it does look to me like we could probably do without it. Thanks for your input. I'll look into it a bit more, then decide
[15:03:48 CST(-0600)] <cindyli> ya, michelled, i'm totally fine if you guys decide to go with fireChangeRequest()
[15:34:43 CST(-0600)] <michelled> anastasiac, I know that we discussed making the controlTypes constants but there's something I can't quite put my finger on that is making me uncomfortable with this change
[15:35:10 CST(-0600)] <michelled> Bosmon: can you cast your eye on this pull request? https://github.com/fluid-project/videoPlayer/pull/4/files
[15:35:29 CST(-0600)] <Bosmon> Thanks, michelled
[15:35:50 CST(-0600)] <Bosmon> To recap part of a conversation we had in private, we talked i) about the role of "string constants in JavaScript"
[15:36:27 CST(-0600)] <Bosmon> I expressed my growing feeling that trying to externalise these into a kind of Java-esque centralised table is much more worrisome in JavaScript
[15:36:52 CST(-0600)] <Bosmon> Although it still has some things going for it, for example in terms of being more self-documenting, and reducing the risk of mistyping
[15:37:33 CST(-0600)] <Bosmon> In this particular case, a) the grabbing of the global name "fluid.controls" is problematic, from the point of view of something which should be really scoped to something like "fluid.videoPlayer"
[15:38:03 CST(-0600)] <Bosmon> b) having a single structure of the form "fluid.controls" at all is problematic from the point of view of modularity - if someone wants to extend the videoPlayer, they would need to find and bash on this central structure
[15:38:29 CST(-0600)] <Bosmon> But it seems the best way to resolve all of these problems is to ii) turn these "named pieces of logic" into subcomponents, rather than indexing them by public string constants
[15:38:43 CST(-0600)] <lahabana> michelled: just done a pull request for FLUID-4557
[15:38:58 CST(-0600)] <Bosmon> At least, the string constants then become the standard kind of public names that we have for subcomponents, which resolves that part of the issue
[15:38:59 CST(-0600)] <lahabana> michelled: these are not big modifications
[15:39:06 CST(-0600)] <Bosmon> And then the extension issue takes care of itself too
[15:39:23 CST(-0600)] <lahabana> michelled: but needed to be done if you have anything to say about it mail me
[15:39:32 CST(-0600)] <lahabana> bye everybody
[15:39:42 CST(-0600)] <Bosmon> So, rather than have a block of logic which indexes various strategies by testing against string constants, each alternative should be encapsulated as an IoC configured subcomponent of the overall component
[15:40:29 CST(-0600)] <colinclark> Bosmon, michelled, anastasiac: These are excellent review points
[15:40:47 CST(-0600)] <colinclark> very helpful
[15:40:54 CST(-0600)] <colinclark> and they make a ton of sense to me
[15:41:16 CST(-0600)] <michelled> yes, this is a good and clear strategy
[15:41:28 CST(-0600)] <Bosmon> As a wider review point, I would recommend factoring out the entirety of "bindKeyboardControl" into a block of configuration
[15:41:35 CST(-0600)] <Bosmon> Rather than a method which produces configuration
[15:42:15 CST(-0600)] <Bosmon> Similarly, the "defaultKeys" structure on line 99 should become a globally visible configuration block
[15:42:50 CST(-0600)] <Bosmon> It looks like this whole "controls" concept is a bit "ant-like" (in terms of our ideas about UIOptions) given it is a switch which is used in multiple places
[15:42:57 CST(-0600)] <Bosmon> For example, it appears again in "produceTree"
[15:43:19 CST(-0600)] <Bosmon> A suitable strategy for dealing with that role of it would be to have the base component fire an event, called something like "onProduceTree"
[15:43:36 CST(-0600)] <Bosmon> The new subcomponents we make can have their effect at this time by subscribing to the event and contributing their material to the tree
[15:44:26 CST(-0600)] <Bosmon> Oh wait, I misread.... the ONLY place this is used is inside produceTree
[15:44:28 CST(-0600)] <Bosmon> So, that is fine
[15:45:19 CST(-0600)] <Bosmon> The "diff" view made me think that logic like this is actually part of bindKeyboardControl - but that's actually a completely separate review point
[15:47:15 CST(-0600)] <Bosmon> On line 264, "that.fullscreen" is a suspicious kind of top-level method to have on a component - since it's not actually suitable for user use
[15:48:07 CST(-0600)] <Bosmon> Seeing it there, the user might think that they could call it to change the fullscreen condition of the component, but actually they can't