fluid-work IRC Logs-2012-02-21

Owned by Fluid IRC Log Bot
Last updated: Feb 21, 2012Version comment
10 min read

[12:58:42 CST(-0600)] <Justin_o> yura, michelled: do you know if there is a way to have multiple listeners, but have them fire in a specific order

[12:59:02 CST(-0600)] <yura> Justin_o: ya you need priory

[12:59:06 CST(-0600)] <yura> priority

[12:59:21 CST(-0600)] <Justin_o> yura: so same as in sub component creation?

[12:59:55 CST(-0600)] <yura> ya

[13:00:00 CST(-0600)] <Justin_o> yura: thanks

[13:00:04 CST(-0600)] <yura> np

[13:24:24 CST(-0600)] <michelled> alexn1: have you pushed your most recent commit to github?

[13:25:02 CST(-0600)] <alexn1> michelled: Yes I did

[13:25:20 CST(-0600)] <michelled> can you recap here what stage the uber branch is at?

[13:27:23 CST(-0600)] <alexn1> sure. Let me push the latest change I have here though. Finally, managed to fix a captionator issue when caption cues do not rerender in case of video resize or UIO options appearance. It is a very happy moment for me (smile)

[13:27:40 CST(-0600)] <michelled> (smile)

[13:30:38 CST(-0600)] <alexn1> michelled: branch is here https://github.com/xOZx/videoPlayer/tree/FLUID-4608-merge

[13:31:55 CST(-0600)] <michelled> alexn1: what is still remaining to do in the branch?

[13:32:01 CST(-0600)] <michelled> thanks for the link (smile)

[13:32:32 CST(-0600)] <alexn1> current issues I'm tackling: 1) video maximize/minimize was changed (probably styling as well) it does not work properly anymore. 2) in default color scheme all sliders are not showing up in the mammals page 3) flc-videoPlayer-captionArea is not in use anymore, however I attach captionator cues to the parent div of the video. Maybe it makes sense to attach it to this caption area div (thinking about this) 4) list of remarks I sent to the list

[13:33:43 CST(-0600)] <michelled> just to clarify - is 4) your testing results?

[13:33:58 CST(-0600)] <michelled> or did you send another email that I have missed somehow?

[13:34:25 CST(-0600)] <alexn1> yes michelled. I still have to reply on your email feedback about my remarks with some clarifications about specific points.

[13:34:52 CST(-0600)] <alexn1> no no. There was only 1 with additional smaller email which had like 3 more additional remarks

[13:35:15 CST(-0600)] <michelled> ok, great - those issues will be handled under separate JIRAs

[13:35:47 CST(-0600)] <michelled> I mean the ones that fall under 4)

[13:35:54 CST(-0600)] <alexn1> gotcha

[13:36:18 CST(-0600)] <michelled> I think it makes sense to have captionator use the flc-videoPlayer-captionArea instead of the overall container

[13:36:28 CST(-0600)] <michelled> is there any reason you think we should use the overall container?

[13:37:41 CST(-0600)] <alexn1> I'm not sure yet… was planning to play around with it and see that nothing breaks

[13:37:50 CST(-0600)] <alexn1> will let you know my results

[13:38:40 CST(-0600)] <alexn1> but other first 3 issues are more tricky since I see that 1) was clearly changed and 2) was also probably a change related to the CSS. Need to find out how to fix this

[13:38:41 CST(-0600)] <michelled> alexn1: there is also a JIRA to handle the issues with maximize/minimize.

[13:39:22 CST(-0600)] <alexn1> could you send me the JIRA number because I do not see it in the list

[13:39:29 CST(-0600)] <michelled> alexn1: FLUID-4570

[13:40:11 CST(-0600)] <alexn1> thx!

[13:40:19 CST(-0600)] <michelled> alexn1: we might need to pull the maximize/minimize out of the demo if we can't get to that JIRA in time

[13:40:53 CST(-0600)] <michelled> alexn1: your point 2) above seems to have been introduced by the merge so it will need to be handled in this branch

[13:41:40 CST(-0600)] <alexn1> I am not sure that it was introduced by the merge

[13:42:16 CST(-0600)] <alexn1> because I cannot check mammals page in the branch anastasia left since it does not work there to be sure that it was the merge which resulted in this css effect

[13:43:39 CST(-0600)] <alexn1> anyway, I will investigate this

[13:46:07 CST(-0600)] <michelled> alexn1: it looks like it was already a problem (sad) http://build.fluidproject.org/videoPlayer/videoPlayer/demos/Mammals.html

[13:46:25 CST(-0600)] <michelled> I tested this before I made a pull request but it's clearly broken in the repo

[13:46:46 CST(-0600)] <michelled> alexn1: I should look into it - it was broken when my pull request went in

[13:46:59 CST(-0600)] <michelled> alexn1: I think that means your branch is ready for review

[13:47:01 CST(-0600)] <alexn1> oh ok.

[13:47:07 CST(-0600)] <michelled> Bosmon: any chance you can review alexn1's branch?

[13:47:17 CST(-0600)] <Bosmon> I can have a look at it

[13:47:28 CST(-0600)] <Bosmon> I probably don't have time to do a full review, so I might need some backup (smile)

[13:47:43 CST(-0600)] <michelled> of course

[13:47:44 CST(-0600)] <Bosmon> Although I can look at it fully in the evening

[13:49:01 CST(-0600)] <alexn1> Go team Bosmon! (smile)

[13:50:03 CST(-0600)] <Bosmon> Actually I think I somewhat prefer the days now when I am no longer a team of 1 : P

[13:50:55 CST(-0600)] <alexn1> well ninjas usually go in teams of one. So you could be considered as an Infusion ninja

[14:02:19 CST(-0600)] <michelled> alexn1: I'm tempted to wait until after your branch is ready and then look at anastasiac's high fidelity pull requests. they don't merge cleanly right now and I don't want to change the master again until the uber branch is in

[14:02:39 CST(-0600)] <michelled> alexn1: I have a feeling that some of the styling issues will be addressed by her pull request and other branches that have been starters

[14:02:45 CST(-0600)] <michelled> started

[14:02:52 CST(-0600)] <alexn1> I see

[14:03:17 CST(-0600)] <alexn1> Well then maximize/minimize button + moving captions into the caption div is a priority for me

[14:06:10 CST(-0600)] <alexn1> also on another note - We might want to modify UIO options functionality since it modifies text on the page by changing styling of the page's body tag. Which implies that it does not overwrite the captionator text styling. Short speaking, captionator text does not increase in size as well as text inside any other div/control which has its own styling when you use UIO panel.

[14:08:14 CST(-0600)] <michelled> alexn1: why does the captionator text not change? is it not being sized relative to the body size?

[14:10:43 CST(-0600)] <alexn1> well because a caption cue has its own styling. Throw any control on the page with its own text styling and it won't change since element's styling has a higher priority than the body's styling. Correct me if I'm wrong.

[14:13:24 CST(-0600)] <Bosmon> alexn1 - I can't think that having ninjas is any better than having WIZZARDS (smile)

[14:13:44 CST(-0600)] <Bosmon> They are both pretty annoying kinds of people to have around... perpetually leaping out from behind bushes and startling the children

[14:14:57 CST(-0600)] <michelled> lol

[14:21:20 CST(-0600)] <michelled> alexn1: when the page is styled using relative font sizes then a change to the size on the body should change its size

[14:21:31 CST(-0600)] <michelled> alexn1: take a look at the studios site for an example: http://studios.fluidproject.org/

[14:55:19 CST(-0600)] <Bosmon> hi alexn1, michelled - I don't think there is a pull request yet for the branch I should review?

[14:55:22 CST(-0600)] <Bosmon> Could you point me at it?

[14:56:25 CST(-0600)] <alexn1> well there is no pull request for the branch… not sure if there should be one

[14:56:33 CST(-0600)] <alexn1> but I can point out to the branch in my github

[14:56:33 CST(-0600)] <alexn1> http://issues.fluidproject.org/browse/FLUID-4608

[14:56:46 CST(-0600)] <michelled> alexn1: is that the merge branch?

[14:56:50 CST(-0600)] <michelled> or is this one? https://github.com/xOZx/videoPlayer/tree/FLUID-4608-merge

[14:57:06 CST(-0600)] <alexn1> (sad)

[14:57:07 CST(-0600)] <alexn1> my bad

[14:57:13 CST(-0600)] <alexn1> yes it is https://github.com/xOZx/videoPlayer/tree/FLUID-4608-merge

[14:57:13 CST(-0600)] <michelled> Bosmon: alexn1 and I were just chatting and there are a couple refinements that he's still working on

[14:57:21 CST(-0600)] <michelled> alexn1: can you drop the list in here?

[14:57:28 CST(-0600)] <Bosmon> There should probably be a pull request... and it can replace your existing one for FLUID-4554

[14:57:29 CST(-0600)] <Bosmon> ok

[14:57:30 CST(-0600)] <michelled> other than those, I think the branch is ready for review

[14:57:39 CST(-0600)] <Bosmon> ok, cool

[14:57:52 CST(-0600)] <alexn1> oh.. let me fix the grammar and make my notes more presentable then (smile)

[14:58:06 CST(-0600)] <Bosmon> I think the github mechanism for commenting on pull requests is slightly better than the one for branches?

[14:58:14 CST(-0600)] <Bosmon> Since I think it pools all the comments into one place...

[14:58:21 CST(-0600)] <Bosmon> But I can try commenting on the branch directly

[14:58:44 CST(-0600)] <michelled> ah, in that case perhaps alexn1 should make the pull request

[15:00:00 CST(-0600)] <alexn1> give me 1 minute. I will fix my notes and will submit a pull request

[15:00:00 CST(-0600)] <Bosmon> Wow

[15:00:04 CST(-0600)] <Bosmon> It really is a big merge : P

[15:00:13 CST(-0600)] <michelled> uber branch (smile)

[15:00:46 CST(-0600)] <Bosmon> Although a lot of the bulk is just captionator.js and the CSS....

[15:01:01 CST(-0600)] <Bosmon> God, the captionator is diabolical

[15:01:23 CST(-0600)] <Bosmon> Which features are we using of it?

[15:01:49 CST(-0600)] <alexn1> Things to do:

[15:01:49 CST(-0600)] <alexn1> 1) Add a Readme file, check captionator's license and mention it in our project

[15:01:49 CST(-0600)] <alexn1> 2) Move captionator div into a caption div defined in our videoPlayer. Fix the alignment so that it looks presentable

[15:01:49 CST(-0600)] <alexn1> 3) Change captionator styling to be relative so that we could apply UIO changes to it

[15:01:50 CST(-0600)] <alexn1> 4) Fix mammal case filename case issue

[15:01:50 CST(-0600)] <alexn1> 5*) The last thing in the list is a proper implementation of the maximize/minimize functionality

[15:01:53 CST(-0600)] <alexn1> In a separate branch:

[15:01:53 CST(-0600)] <alexn1> 1) Merge Anastasia's high fidelity branches with the uber branches. (Look at the Anastasia's branches in github)

[15:03:38 CST(-0600)] <michelled> 5 is also in a separate branch

[15:03:38 CST(-0600)] <alexn1> michelled: I should not merge upstream branch into our uber-branch, right ?

[15:03:56 CST(-0600)] <michelled> alexn1: which upstream branch?

[15:04:26 CST(-0600)] <alexn1> master branch from this repo - https://github.com/fluid-project/videoPlayer

[15:04:41 CST(-0600)] <alexn1> master branch from the upstream remote

[15:05:05 CST(-0600)] <michelled> alexn1: yes, that should be merged int

[15:05:21 CST(-0600)] <michelled> alexn1: but it must be if you have the mammals demo

[15:05:43 CST(-0600)] <alexn1> well I fixed the mammal demo

[15:06:06 CST(-0600)] <alexn1> it uses new model and a new videoPlayer structure

[15:06:27 CST(-0600)] <Bosmon> It's looking pretty good

[15:06:45 CST(-0600)] <michelled> alexn1: is there anything in the upstream master that isn't in your uber branch?

[15:06:56 CST(-0600)] <Bosmon> As quite minor comments on the HTML5Captioner:

[15:07:09 CST(-0600)] <Bosmon> i) return values are not significant from lifecycle functions, so

[15:07:19 CST(-0600)] <Bosmon> + if(captions.length === 0) {

[15:07:19 CST(-0600)] <Bosmon> + return false;

[15:07:19 CST(-0600)] <Bosmon> + }

[15:07:23 CST(-0600)] <Bosmon> This could just be "return"

[15:07:40 CST(-0600)] <Bosmon> Although it's not quite clear what condition the component will be in if it has no captions...

[15:08:27 CST(-0600)] <Bosmon> + if (currentCaptions.length === 0) {

[15:08:28 CST(-0600)] <Bosmon> + //that.model.currentCaptions.push(0);

[15:08:28 CST(-0600)] <Bosmon> + that.applier.requestChange(elPaths.currentCaptions, [0]);

[15:08:28 CST(-0600)] <Bosmon> + }

[15:08:35 CST(-0600)] <Bosmon> This one could be replaced by just giving a default initial value to the model

[15:09:00 CST(-0600)] <alexn1> michelled: I have no idea. I took my captionator branch and merged it with Anastasia's branch. Did not touch a master branch in upstream since while I was merging it got updated I think

[15:09:30 CST(-0600)] <Bosmon> The "elPaths" idea is an interesting one... but I'm not sure why the user might want the model layout to be configurable in this way

[15:10:11 CST(-0600)] <alexn1> Bosmon: elPaths was a yura's idea to improve my code _

[15:10:23 CST(-0600)] <Bosmon> For example, this var currentCaptions = fluid.get(that.model, elPaths.currentCaptions) || []; - could just be that.model.currentCaptions

[15:10:28 CST(-0600)] <Bosmon> Well, it's an interesting idea (smile)

[15:10:50 CST(-0600)] <Bosmon> But the benefit to it isn't completely clear to me

[15:10:59 CST(-0600)] <Bosmon> Could either you or yura describe it?

[15:12:04 CST(-0600)] <Bosmon> This line contains a bug:

[15:12:05 CST(-0600)] <Bosmon> tracks[key].mode = captionator.TextTrack[!($.inArray(key, currentCaptions)) ? "SHOWING" : "OFF"];

[15:12:27 CST(-0600)] <Bosmon> since inArray returns -1 in the case the index is not found, and 0 is a valid index value

[15:12:34 CST(-0600)] <Bosmon> However, only "0" is considered falsy

[15:13:18 CST(-0600)] <alexn1> Bosmon: actually it works (smile) but I will double check. I just need a bit of time since preparing a pull request here

[15:13:30 CST(-0600)] <Bosmon> I would tend to rewrite it as tracks[key].mode = captionator.TextTrack[($.inArray(key, currentCaptions)) === -1 ? "OFF": "SHOWING"];

[15:13:53 CST(-0600)] <Bosmon> Well, if it works, it could only work as a result of another bug : P

[15:14:52 CST(-0600)] <Bosmon> You have a similar bug on line 108: if (!($.inArray(key, fluid.get(that.model, elPaths.currentCaptions)))) {

[15:24:35 CST(-0600)] <alexn1> Bosmon: I'm running this in a Firebug ->

[15:24:46 CST(-0600)] <alexn1> !($.inArray(0, [0,1])) returns True

[15:24:54 CST(-0600)] <alexn1> !($.inArray(5, [0,1])) returns False

[15:25:03 CST(-0600)] <alexn1> !($.inArray(0, [0])) returns True

[15:25:10 CST(-0600)] <alexn1> !($.inArray(0, [1])) returns False

[15:25:18 CST(-0600)] <alexn1> this code worked for me for a long time already (tongue)

[15:25:26 CST(-0600)] <alexn1> *has been working

[15:31:54 CST(-0600)] <Bosmon> alexn1 - yes, but your set of examples is faulty

[15:32:07 CST(-0600)] <Bosmon> Since it doesn't contain the test !($.inArray(1, [0,1]))

[15:32:12 CST(-0600)] <Bosmon> What value would you expect for this

[15:32:15 CST(-0600)] <Bosmon> And which value do you get (smile)

[15:33:49 CST(-0600)] <alexn1> false

[15:33:51 CST(-0600)] <alexn1> interesting

[15:34:45 CST(-0600)] <alexn1> it works for !($.inArray(1, [1])) returning True

[15:35:03 CST(-0600)] <Bosmon> Sure

[15:35:10 CST(-0600)] <alexn1> but you are right, my friend

[15:35:14 CST(-0600)] <alexn1> the logic is incorrect there

[15:35:33 CST(-0600)] <Bosmon> As I explained, only a 0 value is considered falsy... which, rather than representing "not found" as a $.inArray return, actually represents "found at index 0"

[15:35:36 CST(-0600)] <alexn1> because once we have multiple captions on a screen this code stops working

[15:35:52 CST(-0600)] <Bosmon> So you have no subsitute for an explicit === -1 check

[15:50:44 CST(-0600)] <alexn1> Bosmon, michelled: I submitted a pull request with the uber branch. You can start reviewing code while I will be addressing some of the issues mentioned earlier in the list.

[15:56:43 CST(-0600)] <Bosmon> Thanks, alexn1 - I'm rushing off now, but will look over the request in the evening

[15:56:54 CST(-0600)] <alexn1> thanks Bosmon!

[15:57:11 CST(-0600)] <alexn1> always glad to read your feedback and suggestions