kodacapo
commented
9 months ago On Oct 10, 2023, at 20:57, Jorge Aranda @.***> wrote:
@jorgearanda commented on this pull request.
This is looking soo good! First pass to just clear out some minor things; I haven't gone through the new logic in detail just yet. Thanks for all of this! 🥇
Thank you, but apologies for the (more than I thought) miscellaneous garbage still lying around! I’ve been meaning to clean up for a couple months - that’s why the PR was so slow in coming. I finally just went for it. :-)
Comments below.
@@ -1,15 +1,25 @@ {
- "name": "fishsim",
- "version": "4.1.0",
- "name": "rmkfish",
- "version": "0.1.0",
We should probably stick with fishsim, and bump either the major version (to 5.0.0) or the minor (to 4.2.0)?
FishSim is fine by me. How about 5.1, since Bob’s last Fish manual says version 5, and this now contains stuff that’s not in that manual.
"private": true, "repository": { "type": "git",
- "url": "https://github.com/jorgearanda/fish"https://urldefense.com/v3/__https://github.com/jorgearanda/fish*22__;JQ!!CGUSO5OYRnA7CQ!bYfYGNXHHQCNA3kXObNGnNcDasDr4NOUpuLck12JS1F4QkPcjysCiehc2b59f65vtQG0aEOlnKU-vIz1ll5hCgGd-eblgQ$
- "url": "https://github.com/kodacapo/rmkfish"https://urldefense.com/v3/__https://github.com/kodacapo/rmkfish*22__;JQ!!CGUSO5OYRnA7CQ!bYfYGNXHHQCNA3kXObNGnNcDasDr4NOUpuLck12JS1F4QkPcjysCiehc2b59f65vtQG0aEOlnKU-vIz1ll5hCgFv9dYq9g$
I think this should stay as jorgearanda when merging to this repo
Agree.
},
- "scripts": {
- "build": "babel src -d dist && babel public/js -d dist/public/js",
- "nodemonConfig": {
- "verbose": true,
- "watch": ["./dist/app.js"],
- "delay": 5000
- },
- "scripts": {
Nit: whitespace is off on this line?
Agree. No idea why.
In public/js/basic.test.jshttps://urldefense.com/v3/__https://github.com/jorgearanda/fish/pull/256*discussion_r1353696928__;Iw!!CGUSO5OYRnA7CQ!bYfYGNXHHQCNA3kXObNGnNcDasDr4NOUpuLck12JS1F4QkPcjysCiehc2b59f65vtQG0aEOlnKU-vIz1ll5hCgHeRikLCw$:
- return done();
- });
- });
- describe('The web client', () => {
- // it('should respond to pings', done => {
- // request(app)
- // .get('/ping')
- // .expect(200)
- // .end(function(err, res) {
- // assert(err === null, err);
- // assert(res.text === 'pong', 'The app did not return with a
pong');- // return done();
- // });
- // });
- });
This describe block should probably go away, if the test is commented 🙂
Agree.
In public/js/default-text.jshttps://urldefense.com/v3/__https://github.com/jorgearanda/fish/pull/256*discussion_r1353699750__;Iw!!CGUSO5OYRnA7CQ!bYfYGNXHHQCNA3kXObNGnNcDasDr4NOUpuLck12JS1F4QkPcjysCiehc2b59f65vtQG0aEOlnKU-vIz1ll5hCgGdOk7OcQ$:
\ No newline at end of file +var endDepletedText = 'All the fish are now gone.'; + +var catchIntentPrompt1 = 'How many fish do you plan to catch in the next season?'; +var catchIntentPrompt2 = '(Optional, and we won\'t hold you to it!)'; + +// REDIRECTION FEATURE + +var explainRedirectText = '
FISH REDIRECTION FEATURES
' +
- 'If this FISH experiment is part of a chain, you will want to sent participants into FISH ' +
Nit: sent -> send
Agree.
In public/js/default-text.jshttps://urldefense.com/v3/__https://github.com/jorgearanda/fish/pull/256*discussion_r1353702504__;Iw!!CGUSO5OYRnA7CQ!bYfYGNXHHQCNA3kXObNGnNcDasDr4NOUpuLck12JS1F4QkPcjysCiehc2b59f65vtQG0aEOlnKU-vIz1ll5hCgHWVKAIzA$:
\ No newline at end of file +var endDepletedText = 'All the fish are now gone.'; + +var catchIntentPrompt1 = 'How many fish do you plan to catch in the next season?'; +var catchIntentPrompt2 = '(Optional, and we won\'t hold you to it!)'; + +// REDIRECTION FEATURE + +var explainRedirectText = '
FISH REDIRECTION FEATURES
' +
- 'If this FISH experiment is part of a chain, you will want to sent participants into FISH ' +
- 'without having them log in to FISH in the usual form-based way. ' +
- 'Provide the platform before FISH the standard FISH URL but with 2 query parameters:\n' +
Nit: I found this slightly confusing. How about Provide the source platform with the standard FISH URL...?
Agree. Either "source" or "originating" is much better than what’s there.
- $('#catch-intent-submit').on('click', recordMyCatchIntent);
- myCatchIntentDialogConfigured = true;
- }
- $('#catch-intent-input').val("");
- $('#catch-intent-submit').show();
- $('#catch-intent-dialog-box').show();
- $('#catch-intent-input').trigger('focus'); +}
+function hideCatchIntentDialog() {
- $('#catch-intent-dialog-box').hide(); +}
+function checkCatchIntentDisplay() {
- var season = myCatchIntentSubmitted ? st.catchIntentSeason : st.catchIntentDisplaySeason;
- // console.log('checkCatchIntentDisplay(): ' + ' season=' + season + ' mySeason=' + myCatchIntentDisplaySeason);
Nit: can remove this commented console.log() statement
Agree. All commented source should be dropped. It can be useful while developing to have the original close by, but I’m clearly sloppy (forgetful?) about leaving it around.
+//////////////////////////////////////// + +//controls visibility of both seasonal and overall profit columns in one function for show and one for hide +//for tutorial text, table column heading, and table body + +// Not needed now. Maybe later? +// function showProfitColumns(season) { +// $('#profit-season-header').show(); +// $('#profit-season-th').show(); +// $('#profit-total-header').show(); +// $('#profit-total-th').show(); +// for (var i in st.fishers) { +// $('#f' + i + '-profit-season').show(); +// $('#f' + i + '-profit-total').show(); +// } +// }
I'd err on the side of not including this commented function if it's not needed now 🙂
Agree.
}+
- j++;
Hmm. Would you recall if I had a bug here, or what is the reason this increment is happening outside of the block it used to be in?
No bug. I thought about "fixing" it until I realized the logic is correct. The loop is over all fishers, but this client's fisher ("you") is special: she gets displayed in row 0, all other fishers are displayed in row j. So j starts at 1, and is incremented during every iteration EXCEPT the iteration dealing with the current client’s fisher.
@@ -302,11 +472,17 @@ function hideTutorial() {
function setupOcean(o) { ocean = o;
- // console.log('setupOcean: ocean=' + JSON.stringify(ocean));
Nit: delete?
Agree.
displayRules();loadLabels(); updateCosts(); makeUnpausable(); hideTutorial();
- hideCatchIntentColumn();
- hideCatchIntentDialog();
- if (!ocean.profitDisplayEnabled) { // Profit.vis : if profit column checkbox is disabled, hide all relevant elements
Nit: the code is clear enough, no need for the comment! 🙂
Agree.
@@ -355,6 +531,9 @@ function attemptToFish() {
function beginSeason(data) { st = data;
- // console.log('beginSeason: st.season=' + st.season + ', st.status=' + st.status);
Delete line?
Agree.
@@ -383,7 +562,10 @@ function receiveStatus(data) { drawOcean(); }
-function endSeason() { +function endSeason(data) {
- // console.log('endSeason: st.season=' + st.season + ', st.status=' + st.status + ', data=' + JSON.stringify(data));
Same here, and all the // console.log()s below 🙃
Agree.
@@ -479,11 +706,30 @@ function resizeOceanCanvasToScreenWidth() { }
function startTutorial() {
- // Profit.Vis - is this correct here?
- // HK 2DO: should be shown or hidden before starttutorial() is called
- // if(ocean && ocean.profitDisplayEnabled) { // insert profitDisplayEnabled here or include nested in above?
- // showProfitColumns(0);
- // }
- // else {
- // hideProfitColumns();
- // }
The above comment block probably should be deleted?
Agree.
function main() {
- //hideProfitColumns(); //Prof.Vis CatchIntentions handled differently here than inversion I was working from
Delete?
Agree.
In public/js/localization.jshttps://urldefense.com/v3/__https://github.com/jorgearanda/fish/pull/256*discussion_r1353746184__;Iw!!CGUSO5OYRnA7CQ!bYfYGNXHHQCNA3kXObNGnNcDasDr4NOUpuLck12JS1F4QkPcjysCiehc2b59f65vtQG0aEOlnKU-vIz1ll5hCgHtBrrbRA$:
@@ -75,6 +76,7 @@ pt['status_full'] = 'O grupo que você tentou entrar esta lotado. Por favor, not ko['status_full'] = '참여하고자 하는 그룹이 모두 채워졌습니다. 연구자에게 알려주십시오.';
en['status_getReady'] = 'Get ready! The simulation is about to start.'; +en['status_getReady'] = 'Get ready! The game is about to start.'; // RMK
I think we should probably remove these overrides. Happy to revisit the exact text we want to have on each text, though I should say Bob is insistent to not call this a game, as that brings out some competitiveness in folks that could bias things.
I can see Bob’s point. I made this change at the request of Becca (my daughter, prof at U of Dundee) and one of her PhD students. The word ’simulation’ is all over the code, but appears in very few participant-facing places. One alternative to ’simulation’ might be 'experiment', as in en['login_simulationName'] = 'Experiment code'; which Becca might be ok with. I will ask her. Whatever you decide, if Becca really prefers 'game' (and has a reasonable explanation for it :-), then when the integration is complete I will think about a way to make it settable per experiment.
In public/js/localization.jshttps://urldefense.com/v3/__https://github.com/jorgearanda/fish/pull/256*discussion_r1353747046__;Iw!!CGUSO5OYRnA7CQ!bYfYGNXHHQCNA3kXObNGnNcDasDr4NOUpuLck12JS1F4QkPcjysCiehc2b59f65vtQG0aEOlnKU-vIz1ll5hCgE9GYYSIA$:
@@ -229,6 +232,8 @@ fr['info_location'] = 'Lieu'; pt['info_location'] = 'Local'; ko['info_location'] = '현재 위치';
+en['info_intent'] = 'Plan';
I believe we should provide at least a first stab at this text in the other languages we support. Happy to help with this!
Agree. Here’s two I know: :-)
de['info_intent'] = 'Plan'; fr['info_intent'] = 'Plan';
Sorry, I should have provided 'Plan' for every language in the system, as leaving it out will probably cause a null point exception when this is run in a foreign country. :-(
+// } + +// if (app.settings.env === 'development') { +// process.env.NODE_ENV = 'development'; +// logger.transports.Console.level = 'debug'; +// app.use(morgan('dev')); +// app.use(errorHandler({ dumpExceptions: true, showStack: true })); +// } else if (app.settings.env === 'production') { +// var loggerStream = { +// write: function(message) { +// logger.info(message.slice(0, -1)); +// }, +// }; + +// app.use(morgan({ stream: loggerStream })); +// }
Delete the above block?
Agree.
In src/engine/ocean-manager.jshttps://urldefense.com/v3/__https://github.com/jorgearanda/fish/pull/256*discussion_r1353750376__;Iw!!CGUSO5OYRnA7CQ!bYfYGNXHHQCNA3kXObNGnNcDasDr4NOUpuLck12JS1F4QkPcjysCiehc2b59f65vtQG0aEOlnKU-vIz1ll5hCgH9nB-85g$:
this.oceans[oId].microworld.name +' ' + oId + ' (' + this.oceans[oId].microworld.experimenter.username + ')'
- );
- this.deleteOcean(oId);
- );
- this.deleteOcean(oId);
- }
- else { // .purgeScheduled is undefined
- /*
- [JKoomen] Wait with the actual purge until the next scheduled run of this function.
- Since this function runs on a fixed schedule, it could happen that the function runs
- just a split second after some ocean is declared removable in response to some event.
- BUT, events can arrive out of order, especially if there are non-trivial delays due
- to, say, a FISH client in Europe sending an event (socket message) to a FISH server in the USA.
- SO, waiting to do the actual purge until the next cycle gives all the ocean's events
- at least PURGE_INTERVAL msecs to arrive and be acted on without null pointer exceptions.
- */
Nice catch!
That was a tricky one!
— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/jorgearanda/fish/pull/256*pullrequestreview-1669426662__;Iw!!CGUSO5OYRnA7CQ!bYfYGNXHHQCNA3kXObNGnNcDasDr4NOUpuLck12JS1F4QkPcjysCiehc2b59f65vtQG0aEOlnKU-vIz1ll5hCgGGZnFnIQ$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ABRJ7CPALKD2F5BEE7EXDH3X6XVJHAVCNFSM6AAAAAA52YY52KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMNRZGQZDMNRWGI__;!!CGUSO5OYRnA7CQ!bYfYGNXHHQCNA3kXObNGnNcDasDr4NOUpuLck12JS1F4QkPcjysCiehc2b59f65vtQG0aEOlnKU-vIz1ll5hCgEeFQ5XNQ$. You are receiving this because you authored the thread.Message ID: @.***>
Two major new capabilities: asking subjects their intent for the next season, and allowing Fish to be part of a chain of services (recruitment --> Fish --> survey) Also a few display improvements and several bug fixes