Adds command for adding rows

[steve-kasica]

Fixes #1855

Changes proposed in this pull request:

• Adds a command for adding rows by prepending rows to an existing project.
• Adds a new model change for inserting new rows into an existing project.

[wetneb]

Exciting! Congratulations on your first PR to OpenRefine :) Since that's part of a conversation with @tfmorris I'll let him review first. I am withdrawing the corresponding GSoC project proposal since you are quite close to having completed it already. It was probably a bit too light for GSoC anyway.

[tfmorris]

This is awesome! Thank you very much.

I've made a few comments and suggestions, but my biggest question concerns the front end. Do you also have the UI pieces for this or should we create a separate issue to cover those?

For the backend code that's here, most of my comments are minor, but probably the most important is test coverage for the new command.

[steve-kasica]

I don't have UI code written yet, but I'm happy to work on it since I've already started thinking about some edge cases to handle w.r.t the UI. I don't mind keeping the UI work in this PR, and not creating a separate issue since it goes hand-in-hand.

[Ahmed-Elgamel]

Hi @steve-kasica ,I have the UI finished if you want I can push it to this pr. Also, after I added the UI it seems that whatever number of rows I give to the server it only add one row and it adds it to the beginning of the data not at the end.Is this behavior intended?

[tfmorris]

You can address the linter failures with the command ./refine lint.

For the UI, let's keep things simple to start and just do "Add N rows at the beginning" (which was the original use case) and perhaps "Add N rows at the end." More complicated scenarios (other insertion points, non-blank lines, etc) we can leave for future enhancements.

[steve-kasica]

Sorry I must have my GitHub notification settings not setup correctly because I wasn't aware of the activity on this PR from the last couple weeks. I was held up on trying to support multiple cell types in the UI, but @wetneb convinced me to ship what I have at the Meetup today. Anyway, I've pushed those UI changes and will tackle the remaining unresolved changes from @tfmorris tomorrow.

[steve-kasica]

Hi @steve-kasica ,I have the UI finished if you want I can push it to this pr. Also, after I added the UI it seems that whatever number of rows I give to the server it only add one row and it adds it to the beginning of the data not at the end.Is this behavior intended?

@Ahmed-Elgamel Sorry I didn't see this until now. Is there somewhere I can see the UI you've built? I didn't run into those issues with the UI I just pushed, but I bet they stem from serializing new row data from the client and server.

[steve-kasica]

Here's an example for adding blank rows, with the updated input suggested by @magdmartin during the meetup today.

add-blank-rows.mp4

[wetneb]

As I mentioned in the community call yesterday, given that it's your first PR in the project I'd intuitively recommend aiming for something simpler first. The scope proposed by @tfmorris above seemed like a good aim to have:

For the UI, let's keep things simple to start and just do "Add N rows at the beginning" (which was the original use case) and perhaps "Add N rows at the end."

which I understood as "let's not let users input any cell data in this dialog for now".

We can try to work on getting your more ambitious proposal merged but I can imagine that the scale of the review comments and changes required can become too much to handle. To summarize briefly what I'd ask for:

• the UI would need at least styling work to make sure it follows the rest of the look-and-feel of the application, for consistency, and I wouldn't be surprised if more involved design changes come up
• The java tests are quite minimal as it stands, they should do more assertions on the project data to check its contents (beyond looking at row counts)
• this is all without having looked at the actual code in detail, where I expect some changes to be necessary
• nor have I tried it out interactively yet, which generally brings up other sorts of suggested changes :)

If people prefer to stick to the current proposal of letting users input data directly in this dialog, then I can have a go at reviewing this, but want to manage expectations about the follow-up work required that can be significant.

[wetneb]

Hi @steve-kasica, I wonder if your "thumbs up" reaction on the post above means a preference for:

1. sticking to the current scope of the PR (including editing), in which case you would like us to continue reviewing it in this state

OR

2. scaling down to only adding blank rows?

[steve-kasica]

👍 to rolling back to only adding blank rows.

[steve-kasica]

I've added a UI for adding blank rows and finished increasing test coverage. I found a bug when reverting changes when row are appended to the project.

Here's a short demo: https://youtu.be/j4iEtsR0JdM

[wetneb]

Thanks a lot! That looks a lot more manageable to get merged.

Here are a few screenshots to make it easier for others to chime in about the UI:

For the dialog, I'd change the "Submit Query" button to "OK", marking it as primary by adding the button-primary class and position it next to "Cancel" in the same way that we have normalized other dialogs in #6482 (it might be sufficient for that to just rebase your branch on top of the latest master branch).

[wetneb]

I wonder if this class is necessary because since we are only supporting adding blank rows, we should be able to only send the number of blank rows (and not their JSON serialization) to the backend, no?

[steve-kasica]

Yes, we could do that, but I think in the future we would wish that designed this feature from the beginning to send rows that are blank, as opposed to the number of blank rows, see #6461 (comment).

[wetneb]

I wonder if it would make sense to only store the number of additional rows, since they are known to be blank. That would be simpler and would save memory.

On the other hand you could say that it's useful to make it possible to support non-blank rows at the change level for later, so that projects created with later versions of OpenRefine could also be read by earlier versions, but that's generally something that's too hard to support reliably to advertise it to users. Also, if we are to support arbitrary rows, we would probably need to serialize the recon pool too, so that rows can contain recon values.

No strong feelings either way.

[steve-kasica]

I have strong feelings in favor of the current implementation, sending the server rows that are blank. This design decision setups future development for supporting non-blank rows. I think adding rows of new data into an existing project is the primary task that this feature will address and padding a table with blank rows is an edge case, although supporting expressive use of data tables is still important and I understand the pragmatic importance of incremental enhancements in a widely used application such as OpenRefine.

I also think OpenRefine benefits from a new model change for a small subset of new rows, as per this TODO item in MassRowChange. RowAdditionChange begins to address it.

OpenRefine/main/src/com/google/refine/model/changes/MassRowChange.java

Lines 49 to 50 in 183246f

	// TODO: This replaces all rows in the project with a new set of rows, but if only a small percentage of the rows
	// are being changed, it'd be much more efficient to apply a change list of new/modified rows.

[tfmorris]

I'm OK with row data which is required to be blank as long we we're clear about that restriction in the code comments and that the tests match whatever we're claiming to support (I haven't reviewed the tests yet).

[tfmorris]

The dialog button changes look good. I've only done cursory testing so far, but my first attempt generated something weird which I wanted to report. I did: 1) prepend, 2) append, 3) append and got the following change history:

[
  {
    "op": "core/row-addition",
    "rows": [
      {
        "starred": false,
        "flagged": false,
        "cells": []
      }
    ],
    "index": 0,
    "description": "Add rows"
  },
  {
    "op": "core/row-addition",
    "rows": [
      {
        "starred": false,
        "flagged": false,
        "cells": []
      }
    ],
    "index": 1623,
    "description": "Add rows"
  },
  {
    "op": "core/row-addition",
    "rows": [
      {
        "starred": false,
        "flagged": false,
        "cells": []
      }
    ],
    "index": 1629,
    "description": "Add rows"
  }
]

I suspect that this was because I was initially in record mode and switched to row mode between steps 2 & 3, since the project ended up with 1625 records and 1630 rows. Obviously append should use the row count, not the record count.

I'll do some more testing and code review shortly...

[steve-kasica]

Thanks for catching this bug @tfmorris. I can reproduce it. I assumed theProject.rowModel.total would always provide the total rows in a project, even in records mode. The food dataset I was testing on before isn't a good example records-mode data.

I also found another related issue. If I append a row in records mode, it inserts it in the middle of project because it's using record count instead of row count.

If I just replace theProject.rowModel.total with theProject.metadata.rowCount, would that fix that issue?

[wetneb]

My understanding of the status of this PR is that @steve-kasica has responded to reviews. @tfmorris do you still feel responsible for reviewing this or should someone take over?