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

Add menu for joining columns #2110

Merged
merged 4 commits into from Aug 16, 2019
Merged

Add menu for joining columns #2110

merged 4 commits into from Aug 16, 2019

Conversation

msaby
Copy link
Member

@msaby msaby commented Aug 4, 2019

This patch adds a new "Join columns" entry in "Edit column" menu.

image

The dialog box looks like that:

image

To test : create a project with at least 10 columns and different type of values (strings, blank strings, numbers, dates, booleans, null) and play with the options.

Ex:

image

Under the hood it is a wrapper around GREL "join()" function.

For the UI, I did not keep the "traditional" layout of OR made with tables (gave me headache). Instead I used divs and custom css.

For me everything looks ok EXCEPT for the "delete joined columns" option. It seems that this code is wrong. It raises error message in alert box (I put them in comments), as if the successive Refine.postCoreProcess were "treading on there toes". Could someone help please?

columnsToJoin.forEach (function (columnName) {
        // FIXME ERREUR dans Refine.postProcess (project.js l. 359) : 
         // Index: 5, Size: 5
        // Index: 6, Size: 5
        // No column named e
        Refine.postCoreProcess(
          "remove-column", 
          {
            columnName: columnName
          },
          null,
          { modelsChanged: true }
        );
    });
    }

Copy link
Member

@thadguidry thadguidry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for working on this Mathieu! I look forward to more contributions from you in the future!
I've noted a few grammatical changes in the dialog that I think will help.

main/webapp/modules/core/langs/translation-en.json Outdated Show resolved Hide resolved
main/webapp/modules/core/langs/translation-en.json Outdated Show resolved Hide resolved
main/webapp/modules/core/langs/translation-en.json Outdated Show resolved Hide resolved
main/webapp/modules/core/langs/translation-en.json Outdated Show resolved Hide resolved
@thadguidry thadguidry added columns Type: Feature Request Identifies requests for new features or enhancements. These involve proposing new improvements. labels Aug 4, 2019
@msaby
Copy link
Member Author

msaby commented Aug 5, 2019

Thanks! I will submit a new version in a few days.
What annoys me the most is the bug with the successive calls to Refine.postCoreProcess(). When you reapply the history, you can smoothly remove 10 columns or more in less than 1 second. So why a succession of calls in a script is raising errors?

@thadguidry
Copy link
Member

@msaby I don't know why, sorry. Perhaps @wetneb knows.

@msaby
Copy link
Member Author

msaby commented Aug 5, 2019

I also tried to wrap the call with a setTimeout of 2 seconds, but has no effect...

@msaby
Copy link
Member Author

msaby commented Aug 5, 2019

(By the way I also thought of a latter improvement : join ALL columns regardless to their name and number, with a forEach() loop!. This could improve reproducibilty. I think this is the same idea as #1724 )

@wetneb wetneb self-assigned this Aug 5, 2019
@thadguidry
Copy link
Member

thadguidry commented Aug 5, 2019

@msaby maybe we should have an "all" functor. all.cells.value But we handle this via fields currently and so it would look like cells.all.value. I'm not sure what @wetneb has in mind for our support of this idea perhaps even using our existing ColumnGroup to represent that proposed "all" functor ?

@msaby
Copy link
Member Author

msaby commented Aug 5, 2019

@ettorerizza gave this solution in a message to the support list :

forEach(
forEach(row.columnNames, e, row.record.cells[e].value), c, c.join(" ")
).join("|")

@msaby
Copy link
Member Author

msaby commented Aug 6, 2019

An other point : Should I use simply var variable instead of self.variable ?

@wetneb wetneb changed the title Init commit Add menu for joining columns Aug 7, 2019
@wetneb
Copy link
Sponsor Member

wetneb commented Aug 7, 2019

For local variables inside a function, I would use var variable.

@msaby
Copy link
Member Author

msaby commented Aug 7, 2019

The removal seems to work with that recursive code. Is it ok for you ?
The array columnName is "shifted" at each step.

      var columnName = "";
      var onJoinError = function(e) {
        alert (e);
      }
      var recursiveRemoval = function() {
        if (columnsToJoin.length > 0) {
          columnName = columnsToJoin.shift();
          Refine.postCoreProcess(
              "remove-column", 
              {
                columnName: columnName
              },
              null,
              { modelsChanged: true },
              { onDone:recursiveRemoval, onError:onJoinError }
            );
          }
       }
      recursiveRemoval ();

@wetneb
Copy link
Sponsor Member

wetneb commented Aug 7, 2019

Nice! I think it could potentially be cleaner to remove them all in one operation (to get a more compact history), but it's not a hill I would die on!

@msaby
Copy link
Member Author

msaby commented Aug 7, 2019

How would you remove them all in one operation @wetneb ?
Here is a new commit with corrections.

@msaby
Copy link
Member Author

msaby commented Aug 8, 2019

I'm trying to reuse code from column-reordering-dialog.js in order to remove all columns in one step.

In the process I found something disturbing :

In the function used for removing columns, I begin to retrieve all the colums of the project :

var columnsToKeep = theProject.columnModel.columns.map (function (col) {return col.name;})

As this function is called in a callback in the "Refine.postCoreProcess("add-column"...)" or the "doTextTransform(column.name,...", I thought theProject.columnModel.columns will contain the column created by "Refine.postCoreProcess("add-column"...)". It seems it is not the case...

I looked at project.js to understand. Refine.postProcess make this POST call :

$.post(
    "command/" + moduleName + "/" + command + "?" + $.param(params),
    body,
    onDone,
    "json"
);

So if I understand well the callback (onDone) is called after the POST request is made, but there is no assurance that the Java backend process is completely executed when the callback is called.

Or maybe am I missing something?

Example :
image

The console.log gives me all the columns in theProject.columnModel.columns after the creation of the "ddd" column, but this column is not listed

@wetneb
Copy link
Sponsor Member

wetneb commented Aug 8, 2019

@msaby I think for short-running operations (basically everything except reconciliation and fetching urls), the HTTP request completes after the operation is fully executed in the backend. But that does not necessarily mean that the browser has fetched the updates entirely! You might need to either trigger these updates manually, or use another function than postCoreProcess (or with another options) to ensure that your callback is triggered only once the UI has been updated.

@msaby
Copy link
Member Author

msaby commented Aug 8, 2019

I have digged in project.js.
In this block of Refine.postProcess we begin by calling custom callbacks, then we update the UI. The new value of theProject is gotten from the backend by Refine.update (which calls createUpdateFunction which calls reinitializeProjectData )

} else {
   if ("onDone" in callbacks) {
     try {
       callbacks.onDone(o);
     } catch (e) {
       Refine.reportException(e);
     }
   }

   if (o.code == "ok") {
     Refine.update(updateOptions, callbacks.onFinallyDone);

     if ("historyEntry" in o) {
       ui.processPanel.showUndo(o.historyEntry);
     }
   } else if (o.code == "pending") {
     if ("onPending" in callbacks) {
       try {
         callbacks.onPending(o);
       } catch (e) {
         Refine.reportException(e);
       }
     }
     ui.processPanel.update(updateOptions, callbacks.onFinallyDone);
   }
 }

So even if I wait 1 day, theProject will not update, but it does not mean the backend has not finished to process the request ;-)
I've seen that there is a callback "onFinallyDone" that seems to behave differently from "onDone". I'm going to try to use it instead. It should be triggered after the UI is updated.

@msaby
Copy link
Member Author

msaby commented Aug 9, 2019

Here is a 3rd commit

As suggested by @wetneb I now remove all columns in one operation.

I also fixed an issue I found with backspace and quotes in column names and separator and replacement character, and added an option for interpreting \n and \t as newlines and tab (same logic as the replacement menu I wrote last year)

Result:

image

image

@msaby msaby marked this pull request as ready for review August 11, 2019 09:07
@wetneb
Copy link
Sponsor Member

wetneb commented Aug 21, 2019

@msaby quick follow up on this: I have found myself using OR for a project and your Join operation was exactly what I needed, thanks a lot! It works great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
columns Type: Feature Request Identifies requests for new features or enhancements. These involve proposing new improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants