Sync50a #407

MysticJay · 2020-12-07T10:43:58Z

SYNC collision handling

Summary: multiple active clients accessing the drive might cause IITC to "forget" recently done changes when polling the drive.

Added a collision detection that will ask the user with which data-set to continue.

Clean copy of PR #403
addressing Issue #394
Status: RC1

McBen · 2020-12-08T08:35:09Z

sry to say but the "comments" stuff is annoying.
I don't say they are bad at all. But please reduce them to a minimum. Only write a comment that is worth to read or when the code need explanation. In most cases a varialbe rename or an extra variable or moving stuff to an extra function can do a far better job.

code is read by programmers

stuff like this doesn't help anybody. But makes the code messy and outdated comments can lead to wrong assumpations.

MysticJay · 2020-12-08T08:53:29Z

@McBen It took (me) hours to understand SYNC as it was hardly commented at all. Yes code is read by programmers, but some are more skilled than others and some "see" the symantics from long constructs like x= a ? b : c; esp if a and b are implemented as functions.

when I read code I endorse evrey single bit of comment the programmer added as this will explain his intention and makes debugging way more easy (for me).

You may call me a bad programmer, but if we want new DEVs the current uncommented code is tough stuff.

McBen

just started to add some comments to the comments :)
Didn't had time for all..that was simply too much.

McBen · 2020-12-08T09:18:34Z

plugins/sync.js

@@ -35,7 +35,7 @@ window.plugin.sync.authorizer = null;
 window.plugin.sync.registeredPluginsFields = null;


Why not comment the real important stuff like this?
What is it for? What's the structure of that variable?

McBen · 2020-12-08T09:20:53Z

plugins/sync.js

+  registeredMap = new plugin.sync.RegisteredMap(options);   // create a new datasructure 
+                                                            // for the calling plugin and


double line comments

some one might (accidently) add a code line between them and split the comment

McBen · 2020-12-08T09:21:43Z

plugins/sync.js

-  plugin.sync.registeredPluginsFields.add(registeredMap);
+  registeredMap = new plugin.sync.RegisteredMap(options);   // create a new datasructure 
+                                                            // for the calling plugin and
+  plugin.sync.registeredPluginsFields.add(registeredMap);   // add it to the main structure


as said above. would a explanlation of "sync.registeredPluginFields" wouldn't tell much more ?

McBen · 2020-12-08T09:22:31Z

plugins/sync.js

 };



-//// RegisteredMap
+//// RegisteredMap *************************************************************************


note: comments like "*************" usually mean: split this here in 2 files

McBen · 2020-12-08T09:29:51Z

plugins/sync.js

+  this.doSync = 'remote';                                   // normal 'sync'
+                                                            // 'no' sync needed
+                                                            // force 'remote' (1st sync)
+                                                            // force 'local'
+                                                            // sync is on 'stop'
+


I don't understand the comment. I assume "sync","no","remote","local","stop" are valid states of doSync ?

maybe better something like - just my guess what they could mean:

Suggested change

this.doSync = 'remote'; // normal 'sync'

// 'no' sync needed

// force 'remote' (1st sync)

// force 'local'

// sync is on 'stop'

/**

* doSync : state of current sync

* = "sync" : normal sync (what ever this means)

* = "no" : sync needed (or "no sync running, start delay" ?)

* = "stop" : sync stopped by user

* = "remote" : get values from remote

* = "local : use local values instead of remote

*/

this.doSync = 'remote';

McBen · 2020-12-08T09:55:07Z

plugins/sync.js

+
+  this.dataStorage.initialize(                              // initalize the dataStorage to gain 
+    this.forceFileSearch,                                   // find the related file on drive
+    assignIdCallback,                                       // onSuccess: 


why is it named "assignIdCallback" ? it only triggers the callback? isn't it?

McBen · 2020-12-08T09:56:24Z

plugins/sync.js

 };

 window.plugin.sync.RegisteredMap.prototype.initialize = function(callback) {
  this.initFile(this.loadDocument);
 };
-
+// prepend UUID to data


Suggested change

// prepend UUID to data

// prepend UUID to data

space

McBen · 2020-12-08T09:59:56Z

plugins/sync.js

+  );
+
+  this.dataStorage.initialize(                              // initalize the dataStorage to gain 
+    this.forceFileSearch,                                   // find the related file on drive


Suggested change

this.forceFileSearch, // find the related file on drive

this.forceFileSearch, // if true: find the related file on drive

But the "initialze" function should explain what the parameters are for. Not the function call.
If using a javadoc style comments most IDE's will display this informations.

McBen · 2020-12-08T10:00:26Z

plugins/sync.js

@@ -162,46 +183,109 @@ window.plugin.sync.RegisteredMap.prototype.loadDocument = function(callback) {
  // this function called when the document is created first time
  // and the JSON file is populated with data in plugin field
  initializeFile = function() {
-    _this.map = {};
+    _this.map = {};                                         // create an empty map


Suggested change

_this.map = {}; // create an empty map

_this.map = {};

McBen · 2020-12-08T10:00:59Z

plugins/sync.js

    $.each(window.plugin[_this.pluginName][_this.fieldName], function(key, val) {
      _this.map[key] = val;
    });

-    _this.dataStorage.saveFile(_this.prepareFileData());
+    _this.dataStorage.saveFile(_this.prepareFileData());    // save the map to drive


Suggested change

_this.dataStorage.saveFile(_this.prepareFileData()); // save the map to drive

_this.dataStorage.saveFile(_this.prepareFileData());

johnd0e · 2020-12-08T10:55:58Z

In general agree with @McBen.
Also: initial message is not enough: "Clean PR fro SYNC50".
What is it all about? Please add some context.

See also https://github.com/IITC-CE/ingress-intel-total-conversion/wiki/Contributing-Code about commit messages style.

MysticJay · 2020-12-26T11:28:11Z

@johnd0e @McBen

In general agree with @McBen.

Commenting SYNC is "work in progress" and some comments might even need to be revised.
I prefer comments that start at the same column (if possible) and do not exceed certain boundary.
IMHO this helps to "understand" the code without the need to constantly jump between code and comments.
The above remarks you made will be taken forward the one or the other way.

Also: initial message is not enough: "Clean PR fro SYNC50".
What is it all about? Please add some context.
See also https://github.com/IITC-CE/ingress-intel-total-conversion/wiki/Contributing-Code about commit messages style.

fixed

McBen · 2020-12-26T13:16:11Z

The proper use of comments is to compensate for our failure to express ourself in code. Note that I used the word failure. I meant it. Comments are always failures. We must have them because we cannot always figure out how to express ourselft without them, but their use is not a cause of celebration.
(Robert C. Martin / Clean Code)

Some comments are required. Some comments might by helpful. But they always cost time to write, read and maintain. They can be boring and in worst case be missleading or wrong.

Style

[..] without the need to constantly jump between code and comments.

right now you're jumping left & right.
Even worse: when reviewing code on a smaller screen (or as side-by-side) you've messed up code or have to scroll left & right

This style of comments are fine when teaching someone to code. You've the pure code on the left and the documention on the right. This way you can focus and explain the code without the distraction of comments.

While developing this is terrible wrong. While developing code & comments should be a unit. I expect a comment is important, has a valid reason to be there and I've to read it. (like function feedGremlin() { // never call it after midnight)

MysticJay · 2020-12-26T13:37:49Z

This style of comments are fine when teaching someone to code. You've the pure code on the left and the documention on the right. This way you can focus and explain the code without the distraction of comments.

Still I prefer that way of explaining the code and if Xelio had done so in 2013 it wouldn't have cost us/me weeks to understand the code and someone else could have taken the job to refurbish it way earlier. Up to now all we received from other DEVs was "SYNC is a mess, I do not understand it, I will not touch it, I use my own code."

SYNC is very "difficult" code and digging out the ideas @Xelio had in 2013 is time consuming, but unavoidable if we want to keep SYNC alive and compatible.

For the time being the comment style will be "Code-Teaching". When the process is done, we can think about dropping, merging or rewriting comments.

johnd0e · 2020-12-26T14:51:11Z

SYNC is very "difficult" code

So may be it's worth some refactoring to make it easier?

Better spent time simplifying things than explaining wh things are so complicated.

MysticJay · 2020-12-26T15:19:30Z

@johnd0e I explained my intention, the plan, the future development and that the remarks are valuable and will be taken forward into the next steps.

If you do not like my changed comments but still feel the issue is valid you are free to cherry-pick the dev-part only or ignore the PR.

McBen · 2020-12-26T17:49:36Z

plugins/sync.js

-  // this function called when the document is loaded
-  // update local data if the document is updated by other (overwrite!)
-  // and adding a timer to further check for updates
+  // this function gets called when the document is loaded


'document' usually reference to window.document . Here we talk about a g-drive file

McBen

some more comments

McBen · 2020-12-26T17:57:56Z

plugins/sync.js

+      // execute the callback functions             
+      if(_this.callback) _this.callback(            // execute the callback functions with
+        _this.pluginName,                           //    pluginName
+        _this.fieldName,                            //    fieldName
+        null,                                       //    null (for backwards compatibility)
+        true                                        //    true = full file load from Drive
+      );


Suggested change

// execute the callback functions

if(_this.callback) _this.callback( // execute the callback functions with

_this.pluginName, // pluginName

_this.fieldName, // fieldName

null, // null (for backwards compatibility)

true // true = full file load from Drive

);

// execute the callback functions

if(_this.callback) _this.callback(

_this.pluginName,

_this.fieldName,

null, // obsolete, for backwards compatibility

true // load complete file

);

McBen · 2020-12-26T18:07:42Z

plugins/sync.js

 // Store RegisteredMap and handle initialization of RegisteredMap
 window.plugin.sync.RegisteredPluginsFields = function(options) {
-  this.authorizer = options['authorizer'];
+  this.authorizer = options['authorizer'];                  // 


empty comment

Suggested change

this.authorizer = options['authorizer']; //

this.authorizer = options['authorizer'];

McBen · 2020-12-26T18:12:51Z

plugins/sync.js

+  clearTimeout(this.timer);                                 // reset the timer
+  if(Object.keys(this.waitingInitialize).length > 0) {      // if queue still not empty
+    this.timer = setTimeout(
+      function() {_this.initializeWorker()}, 10000          // retry in 10 sec


move time constant to extra variable

Suggested change

function() {_this.initializeWorker()}, 10000 // retry in 10 sec

function() {_this.initializeWorker()}, RETRY_TIME

NB: also comment in 318 references this

McBen · 2020-12-26T18:16:35Z

plugins/sync.js

  }
 };
 //// end RegisteredPluginsFields




-//// DataManager
+//// DataManager ***************************************************************************


is a sign for "split this file here"

McBen · 2020-12-26T18:21:09Z

plugins/sync.js

+    function () {        
+      gapi.client.request({
+        path: '/upload/drive/v3/files/'+_this.fileId,
+        method: 'PATCH',                                    // why patch? 


don't ask me

try with PUT https://developers.google.com/drive/api/v2/reference/files/update
Google is likely to translate PATCH by PUT on the route https://www.googleapis.com/upload/drive/v2/files/{fileId}

McBen · 2020-12-26T18:30:07Z

plugins/sync.js

 window.plugin.sync.updateLog = function(messages) {
  $('#sync-log').html(messages);
 };

+// AuthButon that shows IN the dialog


Suggested change

// AuthButon that shows IN the dialog

// Toggle the state of the AuthButton in sync dialog

McBen · 2020-12-26T18:32:40Z

plugins/sync.js

@@ -717,6 +790,7 @@ window.plugin.sync.toggleAuthButton = function() {
  $('#sync-authButton').toggleClass('sync-authButton-dimmed', authed || authorizing);
 };

+// Sync menuitem in the toolbox


"toggle" and "sync" sounds really differnt..one of it might be wrong

McBen · 2020-12-26T18:33:37Z

plugins/sync.js

+  window.plugin.sync.logger = new plugin.sync.Logger(
+    {'logLimit':10, 'logUpdateCallback': plugin.sync.updateLog}
+  );


go the full way

Suggested change

window.plugin.sync.logger = new plugin.sync.Logger(

{'logLimit':10, 'logUpdateCallback': plugin.sync.updateLog}

);

window.plugin.sync.logger = new plugin.sync.Logger({

logLimit: 10,

logUpdateCallback: plugin.sync.updateLog

});

McBen · 2020-12-26T18:35:25Z

plugins/sync.js

-  window.plugin.sync.authorizer = new window.plugin.sync.Authorizer({
-    'authCallback': [plugin.sync.toggleAuthButton, plugin.sync.toggleDialogLink]
-  });
+// create an authorizer structure


keep the empty line as separator

McBen · 2020-12-26T18:37:28Z

plugins/sync.js

+    'authCallback': [plugin.sync.toggleAuthButton,          // functions to execute after 
+                     plugin.sync.toggleDialogLink           // successfull authorization


another good example why this comment format is bad,
if you add another callback this comment get's messed up ... and it's currently hard to read if you're just looking for "toggleDialogLink"

Comments & linebreaks added to many lines to help understand the code

Commenting the code

Conflict detection implemented.

Suburbanno · 2023-08-03T17:12:37Z

@MysticJay Do you intend to continue this contribution? If not, could you close it?

MysticJay linked an issue Dec 7, 2020 that may be closed by this pull request

SYNC - Collision handling #394

Open

MysticJay added the plugin label Dec 7, 2020

MysticJay mentioned this pull request Dec 7, 2020

Sync050 #403

Closed

MysticJay added the enhancement New feature or request label Dec 7, 2020

McBen reviewed Dec 8, 2020

View reviewed changes

modos189 force-pushed the master branch 3 times, most recently from a34b800 to 7b9edd5 Compare December 12, 2020 07:21

McBen reviewed Dec 26, 2020

View reviewed changes

MysticJay added 3 commits July 13, 2021 20:17

Comments 1

dfe0eb4

Comments & linebreaks added to many lines to help understand the code

Comments 2

bc371bd

Commenting the code

Conflict detection

c9a3415

Conflict detection implemented.

MysticJay force-pushed the SYNC50a branch from f04e92e to c9a3415 Compare July 13, 2021 18:18

MysticJay marked this pull request as draft August 3, 2023 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync50a #407

Sync50a #407

MysticJay commented Dec 7, 2020 •

edited

Loading

McBen commented Dec 8, 2020

MysticJay commented Dec 8, 2020

McBen left a comment •

edited

Loading

McBen Dec 8, 2020

McBen Dec 8, 2020

McBen Dec 8, 2020 •

edited

Loading

McBen Dec 8, 2020

McBen Dec 8, 2020

McBen Dec 8, 2020

McBen Dec 8, 2020

McBen Dec 8, 2020

McBen Dec 8, 2020

McBen Dec 8, 2020

johnd0e commented Dec 8, 2020

MysticJay commented Dec 26, 2020

McBen commented Dec 26, 2020

MysticJay commented Dec 26, 2020

johnd0e commented Dec 26, 2020

MysticJay commented Dec 26, 2020

McBen Dec 26, 2020

McBen left a comment

McBen Dec 26, 2020

McBen Dec 26, 2020

McBen Dec 26, 2020

McBen Dec 26, 2020

McBen Dec 26, 2020

le-jeu Dec 27, 2020

McBen Dec 26, 2020

McBen Dec 26, 2020

McBen Dec 26, 2020

McBen Dec 26, 2020

McBen Dec 26, 2020

Suburbanno commented Aug 3, 2023

		@@ -35,7 +35,7 @@ window.plugin.sync.authorizer = null;
		window.plugin.sync.registeredPluginsFields = null;

		registeredMap = new plugin.sync.RegisteredMap(options); // create a new datasructure
		// for the calling plugin and

	this.forceFileSearch, // find the related file on drive
	this.forceFileSearch, // if true: find the related file on drive

	_this.dataStorage.saveFile(_this.prepareFileData()); // save the map to drive
	_this.dataStorage.saveFile(_this.prepareFileData());

	this.authorizer = options['authorizer']; //
	this.authorizer = options['authorizer'];

	function() {_this.initializeWorker()}, 10000 // retry in 10 sec
	function() {_this.initializeWorker()}, RETRY_TIME

	// AuthButon that shows IN the dialog
	// Toggle the state of the AuthButton in sync dialog

-  window.plugin.sync.logger = new plugin.sync.Logger(
-    {'logLimit':10, 'logUpdateCallback': plugin.sync.updateLog}
-  );
+  window.plugin.sync.logger = new plugin.sync.Logger({
+      logLimit: 10,
+      logUpdateCallback: plugin.sync.updateLog
+  });

		'authCallback': [plugin.sync.toggleAuthButton, // functions to execute after
		plugin.sync.toggleDialogLink // successfull authorization

Sync50a #407

Are you sure you want to change the base?

Sync50a #407

Conversation

MysticJay commented Dec 7, 2020 • edited Loading

McBen commented Dec 8, 2020

MysticJay commented Dec 8, 2020

McBen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

McBen Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnd0e commented Dec 8, 2020

MysticJay commented Dec 26, 2020

McBen commented Dec 26, 2020

MysticJay commented Dec 26, 2020

johnd0e commented Dec 26, 2020

MysticJay commented Dec 26, 2020

Choose a reason for hiding this comment

McBen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Suburbanno commented Aug 3, 2023

MysticJay commented Dec 7, 2020 •

edited

Loading

McBen left a comment •

edited

Loading

McBen Dec 8, 2020 •

edited

Loading