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.

Sign up for GitHub

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

Jump to bottom

Use git diff-tree for DiffFileTree on diff pages #33514

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use git diff-tree for DiffFileTree on diff pages #33514

wants to merge 1 commit into from

Conversation

McRaeAlex
Copy link
Contributor

@McRaeAlex McRaeAlex commented Feb 6, 2025 •
edited
Loading

Modify Diff View FileTree to show all files

Changes

  • removes Show Status button on diff
  • uses git diff-tree to generate the file tree for the diff
  • doesn't reload the diff tree each time we load more files in the preview
  • selecting and unloaded file will keep loading until that file is loaded
  • removes DiffFileList.vue and "Show Stats" in diff options

Open Questions

  • selecting and unloaded file will keep loading until that file is loaded. Is this behaviour okay? It matches what github does.

Demo

In this demo I set git.MAX_GIT_DIFF_FILES=1 in my app.ini to demonstrate a worst case example. In most cases the behaviour isn't nearly as jarring as we load a bunch of files at a time.

Screen.Recording.2025-02-06.at.6.51.03.PM.mov

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 6, 2025
@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 6, 2025
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/frontend labels Feb 6, 2025
@McRaeAlex McRaeAlex changed the title [WIP] Stacked on https://github.com/go-gitea/gitea/pull/33369 Use git diff-tree for DiffFileTree on diff pages Feb 7, 2025
@McRaeAlex McRaeAlex force-pushed the gitea/am/file-tab-diff-tree branch from 4799444 to 5752410 Compare February 7, 2025 03:31
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 7, 2025
@McRaeAlex
Copy link
Contributor Author

@delvh @lunny I taken this out of draft so we can discuss the open question and any other behaviour you want to go over.

@McRaeAlex McRaeAlex marked this pull request as ready for review February 7, 2025 03:38
@lunny lunny requested review from silverwind and kerwin612 February 7, 2025 03:45
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 7, 2025
@lunny lunny requested a review from wxiaoguang February 7, 2025 07:59
@lunny
Copy link
Member

lunny commented Feb 7, 2025

I think you don't need to update all the language files except en_US.

@lunny lunny added this to the 1.24.0 milestone Feb 7, 2025
@McRaeAlex
Copy link
Contributor Author

I think you don't need to update all the language files except en_US.

I am not sure I understand, If I remove it from en_US then why wouldn't I also remove it from every other language?

@lunny
Copy link
Member

lunny commented Feb 7, 2025

I think you don't need to update all the language files except en_US.

I am not sure I understand, If I remove it from en_US then why wouldn't I also remove it from every other language?

Because other language files will be synced from translate.gitea.com, so basicaly you just need to change the en_US file.

@McRaeAlex McRaeAlex force-pushed the gitea/am/file-tab-diff-tree branch from b50105b to 0982492 Compare February 8, 2025 00:27
@lunny
Copy link
Member

lunny commented Feb 8, 2025

I did a test manually, looks like a modified file can not be selected.

@McRaeAlex
Copy link
Contributor Author

@lunny Apologizes for the delay.

I wasn't able to reproduce your issue. Could you provide some more details?

Screen.Recording.2025-02-24.at.1.47.47.PM.mov

@McRaeAlex McRaeAlex force-pushed the gitea/am/file-tab-diff-tree branch from 0982492 to 705f380 Compare February 24, 2025 22:00
delvh
delvh reviewed Feb 24, 2025
View reviewed changes
@McRaeAlex McRaeAlex force-pushed the gitea/am/file-tab-diff-tree branch from 705f380 to dbd2c36 Compare February 25, 2025 00:18
wxiaoguang
wxiaoguang reviewed Feb 25, 2025
View reviewed changes
web_src/js/features/repo-diff.ts
@@ -184,8 +184,6 @@ export async function loadMoreFiles(url: string) {
// the response is a full HTML page, we need to extract the relevant contents:
// 1. append the newly loaded file list items to the existing list
$('#diff-incomplete').replaceWith($resp.find('#diff-file-boxes').children());
// 2. re-execute the script to append the newly loaded items to the JS variables to refresh the DiffFileTree
$('body').append($resp.find('script#diff-data-script'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the legacy script#diff-data-script won't be executed again anymore, these JS code should be moved from "tmpl" to "ts" files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what you mean, the script embedded in the template still runs on first load and needs information from go to template the script.

It could be more minimal such that the template only embeds JSON information however this was also true before this pull request.

Let me know if I have misunderstood. 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be more minimal such that the template only embeds JSON information however this was also true before this pull request.

Actually that was not true before this PR .... the old code was written intentionally to make it could reload the data when "load more", but it seems that the "load more" won't affect the script in tmpl now.

Never mind, I could do some following up changes to refactor the legacy code.

@silverwind silverwind mentioned this pull request Feb 25, 2025
Merged
silverwind added a commit that referenced this pull request Feb 25, 2025
@silverwind
Enable @typescript-eslint/no-use-before-define (#33715)
b35a9da 
The [rule](https://typescript-eslint.io/rules/no-use-before-define/) is
a superset of the eslint base rule, and I tested it with
#33514 (comment)
where it does not raise an error for cyclic types.
@McRaeAlex
Modify Diff View FileTree to show all files
4e74e74 
== Changes

* removes Show Status button on diff
* uses `git diff-tree` to generate the file tree for the diff

update locale to remove diff.show_diff_stats
@McRaeAlex McRaeAlex force-pushed the gitea/am/file-tab-diff-tree branch from dbd2c36 to 4e74e74 Compare February 25, 2025 19:28
@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 25, 2025
@McRaeAlex McRaeAlex requested a review from silverwind February 25, 2025 19:58
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 25, 2025
@wxiaoguang
Copy link
Contributor

Still have question about the script block. Are these variables used? Is the comment correct?

image

@wxiaoguang
Copy link
Contributor

ps: no need to force-push when review starts, the final merge is a squash merge. Force-push makes the review difficult because every-line needs to be re-reviewed since everything is in a commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@delvh delvh delvh left review comments

@silverwind silverwind silverwind approved these changes

@kerwin612 kerwin612 kerwin612 approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/frontend modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/translation size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
1.24.0
Development

Successfully merging this pull request may close these issues.
7 participants
@McRaeAlex @lunny @wxiaoguang @silverwind @kerwin612 @delvh @GiteaBot