Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FLUID-5249, FLUID-4884, FLUID-5506, FLUID-5587, FLUID-5264, FLUID-5616, FLUID-5495 #591

Merged
merged 4 commits into from Aug 13, 2015

Conversation

Projects
None yet
8 participants
@amb26
Copy link
Member

commented Apr 7, 2015

Implementation of "global instantiator model" for framework, new "contextAwareness" implementation replacing final use case for demands blocks, and abolition of numerous obsolete framework features and their uses, including -

  • demands blocks
  • the "old ChangeApplier",
  • "init functions" such as preInit, postInit and finalInit,
  • the "static environment"
  • "grade linkage system"
  • manual subcomponent instantiation, including initSubcomponent, etc.
  • "returnedPath" system
  • obsolete options syntax including @1, @2, COMPONENT_OPTIONS, etc.
  • others
    Initial implementation of "IoC debugging system" capable of inspecting all view-bound grades in an Infusion context (browser pane). Preparing to abolish "eventedComponent" as a separate grade by moving all its materials to "littleComponent". Implementation of "priority constraint system" completing FLUID-5506, allowing "relative priority constraints" of the form "before:namespace", "after:namespace", etc.
FLUID-5458: Updated npmignore
Removed ant related files that no longer exist, also ignoring the preferences framework.
var transRec = instantiator.modelTransactions[transId];
if (!transRec && !instantiator.free) {
var transRec = instantiator && instantiator.modelTransactions[transId];
if (!transRec && instantiator) {

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 29, 2015

Member

We check instantiator twice here. It looks like this could be rewritten as:

if (instantiator && !instantiator.modelTransactions[transId]) { ... }

This comment has been minimized.

Copy link
@amb26

amb26 Jun 29, 2015

Author Member

If the statement were written in that form, we'd then have to duplicate the reference expression to assign a value to transRec which is used in the if statement body and return value

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 29, 2015

Member

Can you bail early in the case of !instantiator instead?

@@ -204,7 +203,7 @@ var demo = demo || {};
* and update the demo's model display.
*/
var setupDataModel = function () {
var applier = fluid.makeChangeApplier(demo.data);
var applier = fluid.makeHolderChangeApplier({model: demo.data});

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 29, 2015

Member

Arguably orthogonal to this pull request, but this Renderer demo is immensely complex and full of code. Presumably a new demo is order some day.

This comment has been minimized.

Copy link
@amb26

amb26 Jul 1, 2015

Author Member

Along with a new renderer :)

(function (){
"use strict";

fluid.defaults("demo.initGridReorderer", {

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 29, 2015

Member

Is initGridReorderer still the appropriate name for this now that it has been defined as a component grade?

@@ -514,6 +513,8 @@ var fluid_2_0 = fluid_2_0 || {};
},
invokers: {
acquireDefaultRange: {
// TODO: problem here - these are dynamic components and so cannot be constructed gingerly

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 29, 2015

Member

Which "these" is this comment referring to? What does the expression "current (old) framework" refer to specifically?

@@ -165,6 +165,13 @@ var fluid_2_0 = fluid_2_0 || {};
expander: {
func: "{that}.computeLayout"
}
},
getRelativePosition: { // an old-fashioned function member

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 29, 2015

Member

It's worth noting here why getRelativePosition is stuck as an "old-fashioned function member" instead of an invoker, and, if possible, what will need to be done in the future to fix this.

@@ -169,10 +175,10 @@ var fluid_2_0 = fluid_2_0 || {};
modelChanged: null
}
});

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 29, 2015

Member

Stray whitespace.

@@ -16,9 +16,17 @@ var fluid_2_0 = fluid_2_0 || {};

(function ($, fluid) {
"use strict";

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 29, 2015

Member

Stray whitespace.

gradeNames: ["fluid.viewComponent", "autoInit"],
gradeNames: ["fluid.viewComponent"],
mergePolicy: {
//"members.queueFiles": "nomerge"

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 29, 2015

Member

Is this here because of the commented-out queueFiles member below and the attached TODO? Is there another way to reflect the needed modelification of this component, rather than leaving commented-out code in?

});

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 29, 2015

Member

Stray whitespace.

@@ -1362,471 +1351,4 @@ var fluid_2_0 = fluid_2_0 || {};
return that;
};


/** OLD CHANGEAPPLIER IMPLEMENTATION (Infusion 1.5 and before - this will be removed on Fluid 2.0) **/

This comment has been minimized.

Copy link
@colinbdclark
* on small array sizes.
*/

fluid.stableSort = function (array, func) {

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 29, 2015

Member

Is this a public-facing API? If so, can you document it?

@@ -818,6 +850,24 @@ var fluid = fluid || fluid_2_0;
fluid.model.composeSegments = function () {
return fluid.makeArray(arguments).join(".");
};

function lastDotIndex(path) {

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 29, 2015

Member

Is this not in a public scope for performance reasons?

@@ -1015,11 +1074,138 @@ var fluid = fluid || fluid_2_0;

var fluid_guid = 1;

/** Allocate an string value that will be very likely unique within this Fluid scope (frame or process) **/
/** Allocate an string value that will be very likely unique within this Infusion instance (frame or process) **/

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 30, 2015

Member

Typo: "an string."


// Fluid priority system for encoding relative positions of, e.g. listeners, transforms, options, in lists

fluid.extremePriority = 4e9; // around 2^32 - allows headroom of 21 fractional bits for sub-priorities

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 30, 2015

Member

Extreme surfing
So extreme!

if (recA.priority.fixed !== null && recB.priority.fixed !== null) {
return recA.priority.fixed - recB.priority.fixed;
} else { // sort constraint records to the end
return (recA.priority.fixed === null) - (recB.priority.fixed === null);

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 30, 2015

Member

Some seriously tricky boolean coercion here. A comment will help.

var defaults = fluid.getGradedDefaults(defaultName);
fluid.doIndexDefaults(defaultName, defaults, index, indexSpec);
}
return index;
};

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 30, 2015

Member

Whitespace, yo.

@@ -1607,7 +1786,7 @@ var fluid = fluid || fluid_2_0;
fluid.makeComponents = function (components) {

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 30, 2015

Member

Can we either mark this as unsupported, document it, or move it to a testing utility?

@@ -1752,8 +1930,7 @@ var fluid = fluid || fluid_2_0;

// A special marker object which will be placed at a current evaluation point in the tree in order
// to protect against circular evaluation
fluid.inEvaluationMarker = {"__CURRENTLY_IN_EVALUATION__": true};
fluid.destroyedMarker = {"__COMPONENT_DESTROYED__": true};
fluid.inEvaluationMarker = Object.freeze({"__CURRENTLY_IN_EVALUATION__": true});

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 30, 2015

Member

❄️ 🏂 🍦 ⛄️

else { // This is hardwired here for performance reasons - no need to protect deeper strategies
target[name] = fluid.inEvaluationMarker;
else {
if (target !== fluid.inEvaluationMarker) { // blatant "coding to the test" - this enables the simplest "re-trunking" in

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 30, 2015

Member

Minor detail: this should be a TODO comment.

// The base grade for fluid.viewComponent and fluid.viewRelayComponent - will be removed again once the old ChangeApplier is eliminated
fluid.defaults("fluid.commonViewComponent", {
gradeNames: ["fluid.littleComponent", "autoInit"],
// The base grade for fluid.viewComponent and fluid.viewComponent - will be removed again once the old ChangeApplier is eliminated

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 30, 2015

Member

This comment is out of date.

@@ -51,9 +41,9 @@ var fluid_2_0 = fluid_2_0 || {};
// NOTE: this function represents a temporary strategy until we have more integrated IoC debugging.
// It preserves the current framework behaviour for the 1.4 release, but provides a more informative
// diagnostic - in fact, it is perfectly acceptable for a component's creator to return no value and
// the failure is really in assumptions in fluid.initComponent. Revisit this issue for 1.5
// the failure is really in assumptions in fluid.initLittleComponent. Revisit this issue for 1.5

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jun 30, 2015

Member

This comment is also out of date.

@amb26

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2015

Thanks @colinbdclark - all review comments so far believed addressed

fluid.defaults("fluid.debug.highlighter", {
gradeNames: ["fluid.viewComponent"],
selectors: {
highlightRoot: "#fluid-debug-highlightRoot"

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jul 2, 2015

Member

Is there a reason why this selector is an ID instead of the standard Fluidic convention of nsamespaced classess?

This comment has been minimized.

Copy link
@amb26

amb26 Jul 12, 2015

Author Member

This container must be unique within the entire document - since it is used to house all highlight nodes even which are destined to hover over elements within nested iframes.

return "highlight-for:" + domId;
};

fluid.debug.highlighter.highlight = function (that, highlightRoot, dispositions) {

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jul 2, 2015

Member

"One function, one responsibility."

This could benefit from being split up into separate functions for creating, styling, and scrolling the highlight element.

return output;
};

fluid.debug.renderDefaults = function (typeName, options) {

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jul 2, 2015

Member

Any reason not enable greater configurability by using stringTemplate() here instead of hardcoding the rendering of default blocks?

if (fluid.isPrimitive(source)) {
if (typeof(source) === "string" && source.indexOf(options.findString) !== -1) {
var path = segs.slice(0, 2);
fluid.set(options.target, path, fluid.copy(fluid.get(options.fullSource, path)));

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jul 2, 2015

Member

Let's assign the value returned from fluid.get() to a variable before copying and setting it.

target: target,
fullSource: options
});
var markup = "<pre>" + JSON.stringify(target, null, 4) + "</pre>";

This comment has been minimized.

Copy link
@colinbdclark

colinbdclark Jul 2, 2015

Member

Again, hardcoded markup.

@michelled

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

Data binding in the renderer example is no longer working.

Steps to reproduce:

  1. Open demos/renderer/
  2. Select the 'Render' button
  3. Change the location
    Note that the location does not change in the 'model' pane.
@simonbates

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

Note: I've been able to reproduce on the build site, so this issue is not limited to this pull request. JIRA filed:

https://issues.fluidproject.org/browse/FLUID-5715

I've found an issue on IE11 on the grid reorderer demo http://tor1-tst-app01.inclusivedesign.ca/demos/reorderer/gridReorderer/

It appears to be limited to IE and I cannot reproduce on FF or Chrome.

To reproduce:

  1. load the demo page
  2. close the overview panel
  3. tab to the letter puzzle
  4. arrow key to "H"
  5. press CTRL + up and then left to move "H" to the top left, release CTRL
  6. press CTRL + right and right again

The bottom right square disappears and we see:

F H A
E B I
D C

It appears to be timing related. If I move slowly it works, if quickly it breaks. Also, if I have the Developer Tools open, I cannot reproduce the problem :(

@michelled

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

The layout reorderer example shows an invalid drop target warning when there is a valid drop target.

Steps to reproduce:

  1. Open demos/reorderer/layoutReorderer/
  2. Using the mouse, pick up the 'Survey Results' panel and hover over the 'The Making of a Thneed' panel.

Note that the warning appears along with the drop target directly below the panel you are hovering over. If you release the panel, it moves to where the drop target was indicated.

@michelled

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

The table of contents does not appear in the table of contents demo. (It is fine in the UIO demo.)

Steps to reproduce:

  1. Open demos/tableOfContents/

Note that there is no table of contents at the top of the page.

@jobara

This comment has been minimized.

Copy link
Member

commented Jul 22, 2015

@amb26 and @acheetham In regards to the self-voicing enactor tests timing out in Safari, I believe that is addressed by changes in the pull request for the mock text-to-speech #612

@amb26

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2015

Hi all - I've updated this branch with fixes to all the regressions listed above which are due to the branch, Simon has JIRAed the IE11 issue at https://issues.fluidproject.org/browse/FLUID-5715 and I JIRAed the CDN issue at https://issues.fluidproject.org/browse/FLUID-5719 . Fixing the preferences demo revealed a core framework bug of https://issues.fluidproject.org/browse/FLUID-5717 which is fixed here, as well as reminding us that FLUID-5615 is still active. As for @acheetham 's issue about the paths of templates in the prefs demo, this seems to be an issue with how that build was deployed since the demo seems to work fine locally - or it may be an issue with the build process itself. For @michelled 's layoutReorderer issue I believe this is "working as designed" - the drop warning is specified to be displayed when we mouse over a locked portlet. It's trunk that has been broken in this respect, possibly for a couple of years, since this behaviour was lost at this point. Ready for another round of review and checking.

@acheetham

This comment has been minimized.

Copy link
Member

commented Jul 29, 2015

The dynamic reorderer manual test at tests/manual-tests/components/reorderer/dynamic/ is producing the following error (which is not happening in master):

Wed Jul 29 2015 10:39:01 GMT-0400 (EDT):
ASSERTION FAILED: Error in assembleCreatorArguments: cannot look up component type name fluid.messageResolver to a component creator grade with an argumentMap
Fluid.js (line 1)
while
instantiating dependent component with name " resolver " with record Object { type="fluid.messageResolver", options={...}} as child of Object { typeName="fluid.reorderer.labeller", id="1triwhdu-51", lifecycleStatus="constructing", more...}
Fluid.js (line 1)
while
instantiating dependent components for component Object { typeName="fluid.reorderer.labeller", id="1triwhdu-51", lifecycleStatus="constructing", more...}

@acheetham

This comment has been minimized.

Copy link
Member

commented Jul 29, 2015

The error described last week for
examples/framework/preferences/conditionalAdjusters-singlePanel/
examples/framework/preferences/usingGrades/
tests/manual-tests/framework/preferences/assortedContent/
tests/manual-tests/framework/preferences/fullPage/
tests/manual-tests/framework/preferences/fullPage-schema/

is still happening (console logs pasted below).

Oddly, it is no longer happening for examples/framework/preferences/conditionalAdjusters/

Wed Jul 29 2015 10:48:44 GMT-0400 (EDT):
ASSERTION FAILED: Error in invoker record: could not resolve members func, funcName or method to a function implementation - got undefined from Object { funcName="fluid.prefs.dataSource.get"}
Fluid.js (line 1)
while beginning instantiation of invoker with name get and record fluid.prefs.dataSource.get as child of Object { typeName="fluid.prefs.store", id="6xqe70dl-33", lifecycleStatus="constructing", more...}
while instantiating dependent components for component Object { typeName="fluid.prefs.store", id="6xqe70dl-33", lifecycleStatus="constructing", more...}
while constructing component of type fluid.prefs.store with arguments [Object { marker={...}, mergeRecords={...}, instantiator={...}, more...}]
while instantiating dependent component with name " settingsStore " with record Object { type="fluid.prefs.store", options={...}} as child of Object { typeName="fluid.prefs.globalSettingsStore", id="6xqe70dl-29", lifecycleStatus="constructing", more...}

@acheetham

This comment has been minimized.

Copy link
Member

commented Jul 29, 2015

In FF, the PrefsFramework demo does not work.

The speech synthesis functionality can't be found, obviously, but the sliding panel fails to open. The only error in the console is "ReferenceError: speechSynthesis is not defined," and the prefs editor seems to get reasonably far along in its instantiation (the "show" button changes its text to the rendered "show display preferences" and when you click it, it changes to "hide") but the panel does not slide down.

@jobara

This comment has been minimized.

Copy link
Member

commented Jul 29, 2015

The UIOptions demo has a console log that says "Binding to document fluid-id-etg7fjr-96". This should probably be removed.

@jobara

This comment has been minimized.

Copy link
Member

commented Jul 29, 2015

The uploader demo throws server response errors (405, 413) when trying to perform an upload. The demo should be simulating an upload, it should not hit a real server, and should succeed at uploading any file that is allowed in the queue.

@maozillah

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2015

Only in IE11 on windows, tests/framework-tests/preferences/html/SelfVoicingEnactor-test.html is throwing an error for "2. the test to be spoken should have been queued correctly", the result is missing
"},
{
"interrupt": false,
"text": "Reading text from DOM"
},
{
"interrupt": false,
"text": "no image"
}"

@acheetham

This comment has been minimized.

Copy link
Member

commented Jul 29, 2015

In Chrome on the Mac, the TextToSpeech component tests run inconsistently:

From all-tests (i.e. http://tor1-tst-app01.inclusivedesign.ca/tests/all-tests.html?testNumber=47), it fails with "Expected at least one assertion, but none were run - call expect(0) to accept zero assertions"

Directly (i.e. http://tor1-tst-app01.inclusivedesign.ca/tests/component-tests/textToSpeech/html/TextToSpeech-test.html) it passes most times, but occasionally "passes" claiming no tests were run because speech is not supported.

@maozillah

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2015

IE11 on windows, tests/manual-tests/framework/preferences/assortedContent
Including the "error in invoker record" error, there is also an IE11 specific error of "CSS3114: @font-face failed OpenType embedding permission check. Permission must be installable" in reference to the OpenSans ttf files.

This error is occurring for all files that require the fonts.

@amb26

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2015

Thanks for the all the reports. I've addressed all of them except for the two IE11 ones from @maozillah - do you think you could issue JIRAs for these? Thanks!
I believe that @acheetham 's TextToSpeech tests are caused by an issue I discovered after merging in @jobara 's pull request #612 - it looks like there is a fault in the interaction of QUnit and QUnit-composite in that their shared test state will become corrupted if the test page waits too long before queuing a test. I have refactored that test impl so that the tests start as soon as possible but the feature detect runs within the test rather than outside it - this seemed to resolve the problems at least on my machine, but it is quite worrying that the feature detect is so slow. Sorry to miss the remainder of @acheetham 's dataSource reports, I hadn't realised that there was a whole paragraph of them in the original report - these should now be all fixed.

@maozillah

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2015

@amb26 will do!

@jobara

This comment has been minimized.

Copy link
Member

commented Jul 30, 2015

The TextToSpeech tests will only run the "Initialization" test in Safari. The others halt. This happens both in the all-tests and when running the test individually.

No errors are reported in the console.

screen shot 2015-07-30 at 1 21 36 pm

@maozillah

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2015

for examples/framework/preferences/usingGrades/
Still getting an console message:
"Uncaught Error: Assertion failure - check console for more details: Error in invoker record: could not resolve members func, funcName or method to a function implementation - got undefined from [object Object]"

@jobara

This comment has been minimized.

Copy link
Member

commented Jul 30, 2015

In the uploader demo, if you attempt to queue a file that exceeds the file size limit ( i believe this is over 20MB, you get a warning. However, clicking on the "Show files" link does not show a list of the files that were not queued.

screen shot 2015-07-30 at 1 20 59 pm

@jobara

This comment has been minimized.

Copy link
Member

commented Jul 30, 2015

@amb26 as per our conversation in the IRC channel. changing the checkTTSSupport method to resolve after onstop instead of onstart seems to address the test issue in Safari.

@amb26

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2015

Thanks for the further testing - all issues so far reported should be fixed.

@cindyli

This comment has been minimized.

Copy link
Member

commented Aug 7, 2015

In Chrome 44 on Mac 10.10.4, loading tests/manual-tests/components/inlineEdit/rich/ throws error:

ASSERTION FAILED: Could not resolve reference tinyMCE to a value

while firing to listener to event named onCreate of component Object {typeName: "fluid.inlineEdit.tinyMCE", id: "5pz8yf-11", lifecycleStatus: "constructed", options: Object, events: Object…}applier: fluid.ChangeAppliercancel: ()container: jQuery.fn.init[1]decorators: Array[1]destroy: ()displayModeRenderer: jQuery.fn.init[1]displayView: Objectdom: Objectedit: ()editContainer: jQuery.fn.init[1]editField: jQuery.fn.init[1]events: ObjectexistingPadding: 0finish: ()id: "5pz8yf-11"isEditing: ()isEditingState: falselifecycleStatus: "constructed"locate: (name,localContainer)model: ObjectmodelRelay: nulloptions: ObjectrefreshView: ()textEditButton: jQuery.fn.init[1]tooltipEnabled: ()typeName: "fluid.inlineEdit.tinyMCE"undo: ObjectupdateModel: ()updateModelValue: ()viewEl: jQuery.fn.init[1]proto: fluid.componentConstructor

while constructing component of type fluid.inlineEdit.tinyMCE with arguments ["#demo-richInlineEdit-container-tinyMCE", Object]

@amb26

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2015

Thanks @cindyli - this issue is already reported at https://issues.fluidproject.org/browse/FLUID-5719

amb26 added some commits Aug 11, 2015

FLUID-5714: Corrections to FLUID-5714 work: correct merging of "direc…
…t string" invokers and listeners which should be interpreted as holding no args entry (rather than empty args entry). Upgrade of fluid.marker system to use immutable prototype objects - these are quicker to detect and now survive our expansion chain unmodified. Final tidying - removed references to Yahoo and opera's loggers, as well as IE8-level logging algorithm.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.