These are chat archives for AxisCommunications/locomote-video-player

25th
Nov 2014
Gaétan Collaud
@gaetancollaud
Nov 25 2014 07:29
I read the code again this morning, for me the workflow is the same. It's just a bit stupid to have a return and not an else... I will quickly correct that
done
Alexander Olsson
@noseglid
Nov 25 2014 07:49
I disagree. Using returns in the middle of functions increases readability because you can sort away cases that are of no interest in your mind. Furthermore, if another condition was required for any reason, it would result in nested ifs which increases the cyclomatic complexity of the function which totally screws readability and also increases the risk of bugs.
I don't really care how you use the player. If you want to contribute I expect you to test and to the utmost of your ability make sure it works with the feature set it provides.
Gaétan Collaud
@gaetancollaud
Nov 25 2014 07:51
Don't you have some integration tests ready to start to validate all features ?
Alexander Olsson
@noseglid
Nov 25 2014 07:51
That said, I suggest you revert 6318b2127b241cdb67e4ac70a9a34c1599485d0d.
No, it's on the todo list. It's not trivial to test a flash application though, especially via travis-ci. I'm planning to getting around to it shortly though.
Gaétan Collaud
@gaetancollaud
Nov 25 2014 07:53
you really want me to revert 6318b2127b241cdb67e4ac70a9a34c1599485d0d ?
Alexander Olsson
@noseglid
Nov 25 2014 07:53
Yes
Normally I would treat it as the code style. But since you brought it up we might as well do it the right way.
Gaétan Collaud
@gaetancollaud
Nov 25 2014 07:56
I reverted
Alexander Olsson
@noseglid
Nov 25 2014 07:59
Right thanks. I'll have a look at it
Alexander Olsson
@noseglid
Nov 25 2014 08:38
@gaetancollaud Thanks. Merged
Gaétan Collaud
@gaetancollaud
Nov 25 2014 08:40
Thank you !!
Alexander Olsson
@noseglid
Nov 25 2014 11:19
@gaetancollaud regarding #114 . The destroy function doesn't appear to support the case where the player does not exist in the DOM. In this case getElementById will fail. I feel what you want to do is store the DOM element (if __embed is called with a string, store document.getElementById('tag') otherwise just store tag. Then do tag.parentNode.removeChild(tag) (rather than just setting the innerHTML to null - which might remove other elements the user put there which have nothing to do with the player).
Also, you should check your commits with git diff --check. Getting a lot of white-space errors.
Gaétan Collaud
@gaetancollaud
Nov 25 2014 12:27
I see. I didn't though the getElementById would fail because we create the element in the constructor. So the id should be in the page. But I will add the check.
I'm ok with the removeChild(tag) too, I didn't thought about the other elements.
The command 'git diff --check' gives me nothing. What do you mean with white-space errors ?
Now that I know that you use Atom, I will do the same and drop Netbeans.
Alexander Olsson
@noseglid
Nov 25 2014 12:37
For instance, if you run git diff 73116d4~1..73116d4 --check (the changes in your commit), Git will tell you about the whitespace errors.
Alexander Olsson
@noseglid
Nov 25 2014 12:45

Since you're doing document.getElementById, if the DOM element is not available in the document it's gonna fail.

for instance

var el = document.createElement('div'); // Creates a new element
el.setAttribute('id', 'myId'); // doing `document.getElementById('myId')` will not work here as it is not in the `document` yet.
var p = new Locomote(el); // Locomote will now attach to `el` but nothing will be visible as `el` is not displayed in the document.
p.destroy(); // This should remove the embedded player regardless if it exists in the DOM or not
document.body.appendChild(el); // Now it exists in the dom,
Gaétan Collaud
@gaetancollaud
Nov 25 2014 12:52
Yep, I see what you mean. I will do that in a moment