fluid-work IRC Logs-2011-11-29

[10:29:46 CST(-0600)] <michelled> anastasiac, cindyli: the more I look at the video player docs the more I feel that they aren't really a read me

[10:30:00 CST(-0600)] <michelled> I'm thinking of leaving them as is for now and writing a new Readme

[10:30:06 CST(-0600)] <michelled> sound ok to you?

[10:30:09 CST(-0600)] <cindyli> ok, michelled

[10:30:27 CST(-0600)] <michelled> I guess eventually anastasiac might want to move these docs into the wiki where the other docs are

[11:30:07 CST(-0600)] <anastasiac> cindyli, I've changed the assignee for your JIRA to you - helps me to keep track

[11:31:11 CST(-0600)] <cindyli> thanks, anastasiac. sorry i forgot to change.

[11:31:22 CST(-0600)] <anastasiac> np

[12:26:12 CST(-0600)] <anastasiac> michelled and cindyli, FYI, I'm starting on FLUID-4547 Video Player code cleanup to get my feet wet

[12:27:18 CST(-0600)] <cindyli> ic. thanks

[12:28:50 CST(-0600)] <anastasiac> michelled and cindyli, if you use the JSONcc caption files and the patch to the HTML that I attached to http://issues.fluidproject.org/browse/FLUID-4542, you will bypass the conversion service altogether, and you should be able to test locally without a web server or PHP

[12:29:36 CST(-0600)] <cindyli> cool. thanks, anastasiac

[12:30:30 CST(-0600)] <anastasiac> well, in firefox, at least, if you have the 'local file security' turned off, michelled and cindyli (I'm not sure how to do that in Chrome)

[12:37:39 CST(-0600)] <anastasiac> michelled and cindyli, I've added a JIRA for adding unit tests to the video player, so if you include any unit tests in what you do, you can include the JIRA number in your commit log: FLUID-4560

[12:37:56 CST(-0600)] <cindyli> sure. thanks

[12:49:56 CST(-0600)] <cindyli> anastasiac & michelled, FLUID-4548 is resolved. shall i hold off on the pull request till michelled's 4542 gets into the master?

[12:50:21 CST(-0600)] <anastasiac> cindyli, I'd guess that's michelled's call

[13:22:03 CST(-0600)] <michelled> cindyli: can you take a quick look at my 4542 branch and make sure my line endings are ok before I push it?

[13:22:59 CST(-0600)] <michelled> anastasiac: I've put in a very basic read me - let me know if you think I should flesh it out now or if you think it will be ok as a placeholder

[13:23:12 CST(-0600)] <cindyli> michelled: sure

[13:23:34 CST(-0600)] <anastasiac> will do, michelled

[13:26:19 CST(-0600)] <anastasiac> michelled, I think the read me will be fine for now. I might add "support" after "described video" just for grammatical consistency with "captions", but then I'm picky

[13:27:04 CST(-0600)] <cindyli> michelled: the line endings look like CRLF. how do you normally check? i pulled out your branch and randomly picked one file to view in eclipse

[13:27:10 CST(-0600)] <anastasiac> michelled, maybe also add a link to the fluid infusion repo?

[13:27:22 CST(-0600)] <anastasiac> or site

[13:28:19 CST(-0600)] <michelled> cindyli: that should be fine - thx for checking

[13:28:28 CST(-0600)] <cindyli> np, michelled

[13:28:30 CST(-0600)] <michelled> anastasiac: I'll fix that

[13:33:08 CST(-0600)] <lahabana> #michelled

[13:33:16 CST(-0600)] <lahabana> @michelled hello

[13:33:42 CST(-0600)] <michelled> hi lahabana - how are you doing?

[13:33:50 CST(-0600)] <lahabana> I'm ok

[13:34:01 CST(-0600)] <lahabana> I finally found time to chat

[13:34:07 CST(-0600)] <lahabana> sorry it took me so long!

[13:34:16 CST(-0600)] <michelled> np

[13:34:26 CST(-0600)] <lahabana> just read your email about the pull request

[13:34:42 CST(-0600)] <michelled> cool - so you've seen what we are up to?

[13:34:55 CST(-0600)] <lahabana> Yes I think

[13:35:02 CST(-0600)] <lahabana> If I understood well

[13:35:28 CST(-0600)] <lahabana> the plan is to remove the core js interactions with the video api

[13:35:35 CST(-0600)] <lahabana> and use mediaElement instead?

[13:36:29 CST(-0600)] <michelled> we are currently thinking of using mediaElement for flash fallback

[13:36:37 CST(-0600)] <michelled> captionator for caption support

[13:36:51 CST(-0600)] <lahabana> ok I see

[13:37:11 CST(-0600)] <michelled> our first step is to do some work on the current video player though

[13:37:30 CST(-0600)] <lahabana> Yes I get quite a few things need to be modified

[13:37:31 CST(-0600)] <michelled> there's a list of JIRAs we are working on here: http://wiki.fluidproject.org/display/fluid/Floe+Iteration+Plan

[13:37:41 CST(-0600)] <lahabana> ok

[13:38:02 CST(-0600)] <michelled> feel free to pick some up and contribute if you want

[13:38:38 CST(-0600)] <michelled> cindyli, anastasiac, lahabana: let's be careful to assign JIRA issues to ourselves before we start working on them so we don't step on each other's toes

[13:38:41 CST(-0600)] <lahabana> yes at least during christmas holliday (2 weeks)

[13:38:53 CST(-0600)] <lahabana> yes ok

[13:39:16 CST(-0600)] <michelled> lahabana: were there other things you were thinking we should do to the player?

[13:39:35 CST(-0600)] <lahabana> I'm reading the issues at the moment

[13:39:52 CST(-0600)] <lahabana> the main thing I was thinking

[13:40:19 CST(-0600)] <lahabana> is that maintaining the core HTML API was quite a lot

[13:40:40 CST(-0600)] <lahabana> and when you were looking to include already made ones

[13:40:46 CST(-0600)] <lahabana> I thought that was a great idea

[13:41:04 CST(-0600)] <lahabana> but apparently you've changed mind?

[13:41:35 CST(-0600)] <michelled> hmmm… well, I think media element will serve that purpose too - perhaps I misunderstood your question earlier

[13:42:52 CST(-0600)] <lahabana> yes I think so

[13:44:57 CST(-0600)] <lahabana> also I still have a few bugs

[13:45:07 CST(-0600)] <lahabana> that Heidi reported me

[13:45:24 CST(-0600)] <lahabana> I will put them on Jira

[13:45:34 CST(-0600)] <lahabana> but I'm not sure if I should

[13:45:50 CST(-0600)] <lahabana> cause as I see it now quite a few things are gonna be really modified

[13:45:58 CST(-0600)] <lahabana> like the appearance

[13:47:03 CST(-0600)] <lahabana> so what do you think michelled?

[13:48:41 CST(-0600)] <michelled> lahabana: the appearance will be completely overhauled so if the bugs are related to appearance then probably don't bother

[13:48:52 CST(-0600)] <Bosmon> cindyli - the way I usually check line endings is to run dos2unix on the file and see if the timestamp changes

[13:48:52 CST(-0600)] <lahabana> ok thx

[13:48:58 CST(-0600)] <michelled> lahabana: but if there are any other bugs you know of, please create JIRAs for them

[13:49:11 CST(-0600)] <lahabana> ok thx

[13:49:29 CST(-0600)] <lahabana> michelled also

[13:49:30 CST(-0600)] <cindyli> Bosmon: ah ha, smart. thanks

[13:49:56 CST(-0600)] <lahabana> if I want to solve an issue

[13:50:03 CST(-0600)] <lahabana> should I make a new fork?

[13:50:11 CST(-0600)] <lahabana> and from what project?

[13:50:36 CST(-0600)] <michelled> lahabana: you can continue to use your fork but you should keep it up to date with the project repo

[13:50:50 CST(-0600)] <lahabana> ok cheers

[13:51:02 CST(-0600)] <michelled> what we usually do is create a branch with the name of the JIRA issue - such as FLUID-4542 and do the work there

[13:51:14 CST(-0600)] <lahabana> ok thx

[13:51:15 CST(-0600)] <michelled> then we do a pull request from that branch

[13:54:53 CST(-0600)] <colinclark> lahabana: The reason we're using MediaElement.js is because it's actually what they call a "Polyfill" for the <video> tag

[13:55:13 CST(-0600)] <colinclark> Meaning, on browsers that don't support the <video> tag, there will be a Flash or Silverlight equivalent

[13:55:20 CST(-0600)] <colinclark> which implements the same API as the <video> tag

[13:55:28 CST(-0600)] <lahabana> ok i see

[13:55:32 CST(-0600)] <colinclark> So, we'll still program against the <video> API, as you've done

[13:55:39 CST(-0600)] <colinclark> it's just that will work in even more browsers

[13:56:28 CST(-0600)] <lahabana> ok I see

[13:56:50 CST(-0600)] <lahabana> I haven't really looked into mediaElement to be honest

[13:57:36 CST(-0600)] <lahabana> also about http://issues.fluidproject.org/browse/FLUID-4557

[13:57:45 CST(-0600)] <lahabana> the changeApplier is created directly

[13:57:50 CST(-0600)] <lahabana> ?

[13:58:15 CST(-0600)] <anastasiac> lahabana, you don't have to create a changeapplier

[13:58:19 CST(-0600)] <lahabana> I can access to change applier by doing that.changeApplier ?

[13:58:26 CST(-0600)] <lahabana> ok thx

[13:58:28 CST(-0600)] <anastasiac> one is automatically created by the framework

[13:58:39 CST(-0600)] <anastasiac> it's attached as that.applier

[13:58:45 CST(-0600)] <lahabana> ok thx

[13:58:59 CST(-0600)] <lahabana> I might do that tonight it doesn't seem to long

[14:04:13 CST(-0600)] <michelled> lahabana: note that I used git rebase on your branch in order to add JIRA numbers to your commits

[14:04:26 CST(-0600)] <lahabana> ho ok

[14:04:28 CST(-0600)] <michelled> so you might actually be better off deleting your fork and reforking

[14:04:32 CST(-0600)] <michelled> it's up to you

[14:04:32 CST(-0600)] <lahabana> so what should I do

[14:04:53 CST(-0600)] <lahabana> to renew my local repo?

[14:05:23 CST(-0600)] <lahabana> ok I might just do that

[14:05:27 CST(-0600)] <michelled> I would reclone your local repo from a new fork

[14:06:39 CST(-0600)] <michelled> lahabana, anastasiac, cindyli: I've pushed the 4542 branch up to the project repo

[14:06:47 CST(-0600)] <anastasiac> yay!

[14:06:49 CST(-0600)] <lahabana> ok thx

[14:06:50 CST(-0600)] <cindyli> cool

[14:06:56 CST(-0600)] <michelled> fluid-everyone: lahabana's gsoc contributions have landed!

[14:07:08 CST(-0600)] <colinclark> yay!

[14:07:14 CST(-0600)] <colinclark> congrats, lahabana

[14:07:20 CST(-0600)] <lahabana> thank u

[14:07:23 CST(-0600)] <colinclark> and thanks to michelled, anastasiac, and cindyli for all the detailed code review

[14:07:39 CST(-0600)] <michelled> next stop a nightly build to show it all off

[14:08:37 CST(-0600)] <lahabana> yes thanks everybody

[14:14:53 CST(-0600)] <cindyli> michelled, anastasiac, i sent a pull requst for 4548: https://github.com/fluid-project/videoPlayer/pull/3

[14:17:14 CST(-0600)] <michelled> cindyli: something looks a little odd with your pull request - it seems to be pulling out the copyright statements

[14:18:04 CST(-0600)] <Bosmon> Hi cindyli - could you explain more of the purpose behind FLUID-4548?

[14:18:11 CST(-0600)] <Bosmon> There doesn't seem to be any text in the JIRA item itself

[14:18:38 CST(-0600)] <Bosmon> I guess since anastasiac is the reporter, this should fall to her

[14:19:59 CST(-0600)] <anastasiac> I was mostly the typist on this one. It was part of the general code review where we noticed that fireChangeRequest() was being used in cases where we thought requestChange() would be more appropriate. Maybe cindyli can add more explanation?

[14:20:56 CST(-0600)] <Bosmon> One might argue that even as typist, you should have refused to submit the issue without a description

[14:21:01 CST(-0600)] <michelled> it's my understanding that fireChangeRequest circumvents the guards - am I wrong on that?

[14:21:48 CST(-0600)] <Bosmon> Perhaps we can add a JIRA hook, a bit like our github commit hook

[14:23:27 CST(-0600)] <colinclark> Bosmon: Perhaps you could elaborate on the difference between fireChangeRequest() and requestChange() on the ChangeApplier?

[14:23:56 CST(-0600)] <colinclark> As opposed to, say, pedantically pointing out the need for better issue descriptions

[14:26:12 CST(-0600)] <anastasiac> michelled, I've filed a pull request for the general code clean-up JIRA: https://github.com/fluid-project/videoPlayer/pull/4

[14:26:38 CST(-0600)] <Bosmon> There is actually no difference

[14:26:44 CST(-0600)] <Bosmon> They are just alternate versions of the same API

[14:27:00 CST(-0600)] <anastasiac> Bosmon, you're right: I should have elaborated. I'm sorry about that. I filed about 6,428 JIRAs and I guess I got sloppy

[14:27:35 CST(-0600)] <michelled> Bosmon: do you prefer one version over another?

[14:29:02 CST(-0600)] <Bosmon> michelled - I guess one of them is more "declarative" and the other is more "concise"

[14:30:25 CST(-0600)] <Bosmon> You can express all kinds of changes in the "declarative" form, whereas the "concise" form is more limited

[14:30:33 CST(-0600)] <Bosmon> I would say it is a matter of taste, based on how the use case is working

[14:30:46 CST(-0600)] <Bosmon> A bit like the different choices about whether to use jQuery.ajax or jQuery.get, etc.

[14:33:02 CST(-0600)] <michelled> thanks Bosmon - I had thought there was an actual difference between them

[14:33:19 CST(-0600)] <michelled> I suppose we don't need to make the change in this case

[14:33:48 CST(-0600)] <michelled> cindyli: do you have a preference for concise over declarative?

[14:37:21 CST(-0600)] <anastasiac> michelled, cindyli, FYI I'm going to tackle the folder restructuring (FLUID-4543)

[14:38:15 CST(-0600)] <anastasiac> michelled, is there a reason you put the caption files in the html folder?

[14:38:52 CST(-0600)] <colinclark> presumably so they can be loaded within the local file ajax restrictions?

[14:40:23 CST(-0600)] <anastasiac> makes sense.

[14:40:35 CST(-0600)] <colinclark> I'm just guessing, for the record

[14:40:54 CST(-0600)] <michelled> well, I didn't try anything else - I was going to do the demo restructuring afterwards and move them to the appropriate place

[14:41:40 CST(-0600)] <anastasiac> ah - do you want to do the restructuring? I was going to start, but...

[14:42:05 CST(-0600)] <anastasiac> michelled: ^

[14:42:13 CST(-0600)] <michelled> no, go ahead

[14:42:30 CST(-0600)] <anastasiac> ok, michelled, thanks. I'm thinking the stuff that's in the utils folder should go in the new lib folder - would you agree?

[14:42:54 CST(-0600)] <michelled> probably - you'll want to check the licenses on those things

[14:43:02 CST(-0600)] <michelled> I should have done that before pushing them in at all

[14:50:26 CST(-0600)] <colinclark> Off the top of my head, I can't remember if it's possible to override a subcomponent's options without also specifying its type...

[14:50:58 CST(-0600)] <anastasiac> lahabana, do you recall where you got two of the 'utils' files, escapeSelector.js and HTML5-backcompat.js ?

[14:51:12 CST(-0600)]

[14:51:22 CST(-0600)] <colinclark> does anyone know?

[14:52:29 CST(-0600)] <colinclark> anastasiac: If I remember correctly, the HTML5-backcompat.js file is from here: https://github.com/jobara/workshops/blob/master/examples/HTML5-inputs/js/HTML5-backcompat.js

[14:52:42 CST(-0600)] <lahabana> anastasiac: yes exactly

[14:52:48 CST(-0600)] <Bosmon> colinclark - yes, you can do that

[14:52:55 CST(-0600)] <Bosmon> Assuming the component is IoC-enabled

[14:52:59 CST(-0600)] <colinclark> Bosmon: Great, thanks

[14:53:24 CST(-0600)] <colinclark> I'm just giving Johnny some instructions on how to work around our not-so-hot UI Options cookie bug

[14:53:42 CST(-0600)] <colinclark> thankfully the cookie expiry is an option of the CookeStore

[14:53:46 CST(-0600)] <colinclark> CookieStore

[14:54:00 CST(-0600)] <Bosmon> I was speculating to michelled earlier about whether this is the level of bug that requires a fresh release

[14:54:04 CST(-0600)] <colinclark> Where did escapeSelector.js come from, lahabana? That one I don't recognize

[14:54:11 CST(-0600)] <colinclark> Bosmon: It's pretty close, yes

[14:54:19 CST(-0600)] <colinclark> I mentioned something to that effect to Johnny today

[14:54:27 CST(-0600)] <colinclark> it's pretty well fatal

[14:54:30 CST(-0600)] <lahabana> hmm let me have a look

[14:54:34 CST(-0600)] <lahabana> where is it precisely

[14:54:36 CST(-0600)] <Bosmon> Yes, it is great that our policy of "configurable by default" saves us, partially

[14:54:38 CST(-0600)] <colinclark> though we should have an awfully simple workaround at least, Bosmon

[14:54:53 CST(-0600)] <Bosmon> But it seems pretty unacceptable that the component actually ships as "broken by default"

[14:54:59 CST(-0600)] <anastasiac> lahabana, it's in the "utils" folder

[14:55:52 CST(-0600)] <lahabana> anastasiac: no I don't remember

[14:55:59 CST(-0600)] <lahabana> think it's of no use anymore

[14:56:31 CST(-0600)] <cindyli> michelled: just saw ur asking of "cindyli: do you have a preference for concise over declarative?"

[14:56:50 CST(-0600)] <anastasiac> lahabana, thanks. I'll check if it's used, and if not, I'll remove it

[14:57:25 CST(-0600)] <cindyli> i personally prefer declarative - requestChange, which is more straightforward and looks cleaner.

[14:58:01 CST(-0600)] <cindyli> i was also wondering if fireChangeRequest is exposed to public since it's actually used by requestChange

[14:58:04 CST(-0600)] <anastasiac> ah, it is used, lahabana...

[14:58:10 CST(-0600)] <lahabana> where about

[14:58:11 CST(-0600)] <lahabana> ?

[14:58:42 CST(-0600)] <anastasiac> VideoPlayer_controllers.js line 577, lahabana

[14:58:59 CST(-0600)] <anastasiac> in createMenuMarkup()

[14:59:11 CST(-0600)] <michelled> cindyli: I think requestChange is the concise version

[14:59:13 CST(-0600)] <cindyli> if they are both public API, i'm totally fine if you guys decide to keep fireChangeRequest() tho

[14:59:24 CST(-0600)] <cindyli> oh, ok, michelled

[14:59:38 CST(-0600)] <cindyli> then my preference is concise

[14:59:57 CST(-0600)] <michelled> cindyli: Bosmon was saying earlier that they actually do the same thing underneath.

[15:00:26 CST(-0600)] <michelled> cindyli: I agree that requestChange is easier to read at a glance

[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