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

[Feature Request] Formatting for CSV Diff #14320

Closed
kdumontnu opened this issue Jan 13, 2021 · 14 comments
Closed

[Feature Request] Formatting for CSV Diff #14320

kdumontnu opened this issue Jan 13, 2021 · 14 comments
Labels
issue/bounty This issue has a bounty associated. Whoever opens a PR and gets it merged can claim the bounty. type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first.
Milestone

Comments

@kdumontnu
Copy link
Contributor

kdumontnu commented Jan 13, 2021

We should apply the same styling from CSV file-view to diffs for CSVs.

It should looks something like this:
image

Bountysource

@kdumontnu
Copy link
Contributor Author

kdumontnu commented Jan 13, 2021

Bounty available!
https://www.bountysource.com/issues/95872476-feature-request-formatting-for-csv-diff

Requirements:

  • Apply the same or similar styling as file-view.tmpl: file-view csv markdown to CSV files in the diff view
  • Add a toggle button to load code vs. render (shown above) with tooltips

Requests:

  • Add summary for "# rows added/deleted, # columns added/deleted" in addition to overall "changes", which already is supported by Gitea
  • If entire row is modified or added/deleted, render the diff left/right instead of top/bottom (see example below)

@techknowlogick techknowlogick added type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first. labels Jan 13, 2021
@lunny
Copy link
Member

lunny commented Jan 13, 2021

But if the diff is to add one column or delete one column. Then every line changed?

@techknowlogick
Copy link
Member

But if the diff is to add one column or delete one column. Then every line changed?

In that case currently it'd show a diff of each line changing, with this ticket it would show in a similar fashion but in a nice formatted way.

@techknowlogick techknowlogick added the issue/bounty This issue has a bounty associated. Whoever opens a PR and gets it merged can claim the bounty. label Jan 13, 2021
@kdumontnu
Copy link
Contributor Author

But if the diff is to add one column or delete one column. Then every line changed?

In that case currently it'd show a diff of each line changing, with this ticket it would show in a similar fashion but in a nice formatted way.

Oh, that's a good question. I think techknologick is right that in the case where an entire row or column is deleted no special case is needed. Also, this should probably use the same config threshold for "diff too large to display" as any other text file.

However, you gave me an idea I'll add above: it would be nice to add a summary at the top: ex. "+3 rows, -2 columns". This all assumes that there are nice, properly-formatted csv files.

I think the other opportunity for improved styling is if an entire column is modified. In that case it would probably be better to show the "diff" to the right, as shown below. However, this is a "nice-to-have" and should not gate merging a PR for this. We can create a separate issue if needed.
image

@thisispalash
Copy link

Hi, I am interested in working on this issue. However, I searched the files on your code base for file-view.tmpl which does not exist. I think the project is doable, so once I know how you want your view, I can get started. Lastly, I want to say a week or so to complete as I also have schoolwork, but I don't think it should be more than that. Let me know if this issue [and bounty] is still open.

@kdumontnu
Copy link
Contributor Author

Hi, I am interested in working on this issue. However, I searched the files on your code base for file-view.tmpl which does not exist. I think the project is doable, so once I know how you want your view, I can get started. Lastly, I want to say a week or so to complete as I also have schoolwork, but I don't think it should be more than that. Let me know if this issue [and bounty] is still open.

Not sure what you mean by searching the files on my code-base, but as far as I know no one has started working on this & bounty is still available! If you're going to work on this please fork from this upstream repo, not my fork.

In order to complete this issue/bounty you'll need to get a PR approved and merged to gitea master. For that you'll have to follow the contribution guidelines & in this case most likely add some unit/integration tests. If you'd like some help getting test cases together I can help.

Good luck - let me know if you have any other questions.

@thisispalash
Copy link

I meant looking for the file file-view.tmpl to understand better what you guys wanted.

I forked it from this branch and will take a look.

Lastly, yeah I think I will need some guidance in testing but I shall contact you then.

@KN4CK3R
Copy link
Member

KN4CK3R commented Feb 6, 2021

I have implemented this feature and improved the rendering a little bit. Currently the toggle button is missing but that should be easy compared to the csv diff.

A simple change:
grafik

Changes in different areas (note the jumping line numbers):
grafik

A removed column is rendered without marking all rows as changed:
grafik

@KN4CK3R
Copy link
Member

KN4CK3R commented Feb 8, 2021

How could the render/source toggle be implemented? Currently I use a display=source query parameter. This works fine for a single file. But if you have a diff with multiple files this would toggle all of them and not just the selected file.

@kdumontnu
Copy link
Contributor Author

How could the render/source toggle be implemented? Currently I use a display=source query parameter. This works fine for a single file. But if you have a diff with multiple files this would toggle all of them and not just the selected file.

Yeah, imo this is an issue with how gitea relies heavily on dynamic serving. I believe you will need to rely on some javascript to toggle the styling on the client side. IMO, this is a prime example of where a proper MVC framework, like Vue would be useful.

One maybe not-so-elegant JS solution would be to render the content both as source and tabular and have the UI button toggle the visibility property of the appropriate element.

A solution in Vue would likely have configurable template styling based on the view.

@KN4CK3R
Copy link
Member

KN4CK3R commented Feb 8, 2021

Sure, the "render both" approach works but should not be preferred. The same problem occurs with my second (not pushed) PR for SVG diffs.

@jtran
Copy link
Contributor

jtran commented Feb 8, 2021

👍 for working on SVG diffs. I started to look into that after implementing #14101, but I wasn't familiar enough with how diffs are displayed, and I got stalled.

How could the render/source toggle be implemented?

I do think that we'd like to have toggling the diff view per file, so I think it will need some JS regardless of how it's done. I don't see a query parameter working.

The two main approaches I've thought about:

  1. Client-side rendering. The server-side template just includes the raw data. When the user toggles, JS uses the data to render the desired view by manipulating the DOM.
  2. Keep the rendering server-side. When the user toggles, JS makes a request to get the desired rendering from the server, which renders a .tmpl. The JS just displays whatever the server gives it.

For client-side rendering, the ease or difficulty of rendering could vary greatly depending on the file type and desired features. For CSV, assuming the server tells the JS what lines changed, this is probably not too complex, but I haven't dug into it.

Also, certain UI responsiveness can only be achieved client-side. Switching between views may not need a remote request at all, which is usually the limiting factor. And the more that's done on the server, the harder it becomes to replace it with something client-side in the future. I'm sure you've considered this, but I want to make sure anyone reading this also takes it into account.

On the other hand, for rendering server-side, I think we would need to add a route to respond with the diff between two commits, of a specific file, with the user's desired view. Maybe like a more granular version of CompareDiff() in the API. If there is any JS that needs to run to initialize the view, we'd need to make sure that runs after load. If this API endpoint doesn't exist already, it'll probably need some refactoring to implement.

Despite needing another route, rendering server-side probably needs fewer changes and is in line with how gitea already does most things.

Forgive me if you've considered all this already; I thought it might help some people to have it written down.

Above, I've tried to be as unbiased as possible. That said, I personally feel that the main thing lacking from the client-side code in gitea is a separation between model and view. Maybe that is a conversation for another day, but I think this is what @kdumontnu is referring to with regard to Vue, and the lack of tools for model-view separation tends to discourage ambitious client-side features.

@KN4CK3R
Copy link
Member

KN4CK3R commented Feb 10, 2021

Some progress:
toggle

PR should be ready this week.

@lunny lunny added this to the 1.15.0 milestone Feb 13, 2021
6543 pushed a commit that referenced this issue Mar 29, 2021
Implements request #14320 The rendering of CSV files does match the diff style.

* Moved CSV logic into base package.

* Added method to create a tabular diff.

* Added CSV compare context.

* Added CSV diff template.

* Use new table style in CSV markup.

* Added file size limit for CSV rendering.

* Display CSV parser errors in diff.

* Lazy read single file.

* Lazy read rows for full diff.

* Added unit tests for various CSV changes.
@kdumontnu
Copy link
Contributor Author

Closed with #14661

@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/bounty This issue has a bounty associated. Whoever opens a PR and gets it merged can claim the bounty. type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

No branches or pull requests

6 participants