diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 817ba3bfaca3..5a2297ac0d3c 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -385,6 +385,8 @@ var migrations = []Migration{ NewMigration("Add allow edits from maintainers to PullRequest table", addAllowMaintainerEdit), // v214 -> v215 NewMigration("Add auto merge table", addAutoMergeTable), + // v215 -> v216 + NewMigration("allow to view files in PRs", addReviewViewedFiles), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v215.go b/models/migrations/v215.go new file mode 100644 index 000000000000..138917edbe77 --- /dev/null +++ b/models/migrations/v215.go @@ -0,0 +1,25 @@ +// Copyright 2022 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "code.gitea.io/gitea/models/pull" + "code.gitea.io/gitea/modules/timeutil" + + "xorm.io/xorm" +) + +func addReviewViewedFiles(x *xorm.Engine) error { + type ReviewState struct { + ID int64 `xorm:"pk autoincr"` + UserID int64 `xorm:"NOT NULL UNIQUE(pull_commit_user)"` + PullID int64 `xorm:"NOT NULL UNIQUE(pull_commit_user) DEFAULT 0"` + CommitSHA string `xorm:"NOT NULL VARCHAR(40) UNIQUE(pull_commit_user)"` + UpdatedFiles map[string]pull.ViewedState `xorm:"NOT NULL LONGTEXT JSON"` + UpdatedUnix timeutil.TimeStamp `xorm:"updated"` + } + + return x.Sync2(new(ReviewState)) +} diff --git a/models/pull/review_state.go b/models/pull/review_state.go new file mode 100644 index 000000000000..59a03c20e8e3 --- /dev/null +++ b/models/pull/review_state.go @@ -0,0 +1,139 @@ +// Copyright 2022 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. +package pull + +import ( + "context" + "fmt" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/timeutil" +) + +// ViewedState stores for a file in which state it is currently viewed +type ViewedState uint8 + +const ( + Unviewed ViewedState = iota + HasChanged // cannot be set from the UI/ API, only internally + Viewed +) + +func (viewedState ViewedState) String() string { + switch viewedState { + case Unviewed: + return "unviewed" + case HasChanged: + return "has-changed" + case Viewed: + return "viewed" + default: + return fmt.Sprintf("unknown(value=%d)", viewedState) + } +} + +// ReviewState stores for a user-PR-commit combination which files the user has already viewed +type ReviewState struct { + ID int64 `xorm:"pk autoincr"` + UserID int64 `xorm:"NOT NULL UNIQUE(pull_commit_user)"` + PullID int64 `xorm:"NOT NULL UNIQUE(pull_commit_user) DEFAULT 0"` // Which PR was the review on? + CommitSHA string `xorm:"NOT NULL VARCHAR(40) UNIQUE(pull_commit_user)"` // Which commit was the head commit for the review? + UpdatedFiles map[string]ViewedState `xorm:"NOT NULL LONGTEXT JSON"` // Stores for each of the changed files of a PR whether they have been viewed, changed since last viewed, or not viewed + UpdatedUnix timeutil.TimeStamp `xorm:"updated"` // Is an accurate indicator of the order of commits as we do not expect it to be possible to make reviews on previous commits +} + +func init() { + db.RegisterModel(new(ReviewState)) +} + +// GetReviewState returns the ReviewState with all given values prefilled, whether or not it exists in the database. +// If the review didn't exist before in the database, it won't afterwards either. +// The returned boolean shows whether the review exists in the database +func GetReviewState(ctx context.Context, userID, pullID int64, commitSHA string) (*ReviewState, bool, error) { + review := &ReviewState{UserID: userID, PullID: pullID, CommitSHA: commitSHA} + has, err := db.GetEngine(ctx).Get(review) + return review, has, err +} + +// UpdateReviewState updates the given review inside the database, regardless of whether it existed before or not +// The given map of files with their viewed state will be merged with the previous review, if present +func UpdateReviewState(ctx context.Context, userID, pullID int64, commitSHA string, updatedFiles map[string]ViewedState) error { + log.Trace("Updating review for user %d, repo %d, commit %s with the updated files %v.", userID, pullID, commitSHA, updatedFiles) + + review, exists, err := GetReviewState(ctx, userID, pullID, commitSHA) + if err != nil { + return err + } + + if exists { + review.UpdatedFiles = mergeFiles(review.UpdatedFiles, updatedFiles) + } else if previousReview, err := getNewestReviewStateApartFrom(ctx, userID, pullID, commitSHA); err != nil { + return err + + // Overwrite the viewed files of the previous review if present + } else if previousReview != nil { + review.UpdatedFiles = mergeFiles(previousReview.UpdatedFiles, updatedFiles) + } else { + review.UpdatedFiles = updatedFiles + } + + // Insert or Update review + engine := db.GetEngine(ctx) + if !exists { + log.Trace("Inserting new review for user %d, repo %d, commit %s with the updated files %v.", userID, pullID, commitSHA, review.UpdatedFiles) + _, err := engine.Insert(review) + return err + } + log.Trace("Updating already existing review with ID %d (user %d, repo %d, commit %s) with the updated files %v.", review.ID, userID, pullID, commitSHA, review.UpdatedFiles) + _, err = engine.ID(review.ID).Update(&ReviewState{UpdatedFiles: review.UpdatedFiles}) + return err +} + +// mergeFiles merges the given maps of files with their viewing state into one map. +// Values from oldFiles will be overridden with values from newFiles +func mergeFiles(oldFiles, newFiles map[string]ViewedState) map[string]ViewedState { + if oldFiles == nil { + return newFiles + } else if newFiles == nil { + return oldFiles + } + + for file, viewed := range newFiles { + oldFiles[file] = viewed + } + return oldFiles +} + +// GetNewestReviewState gets the newest review of the current user in the current PR. +// The returned PR Review will be nil if the user has not yet reviewed this PR. +func GetNewestReviewState(ctx context.Context, userID, pullID int64) (*ReviewState, error) { + var review ReviewState + has, err := db.GetEngine(ctx).Where("user_id = ?", userID).And("pull_id = ?", pullID).OrderBy("updated_unix DESC").Get(&review) + if err != nil || !has { + return nil, err + } + return &review, err +} + +// getNewestReviewStateApartFrom is like GetNewestReview, except that the second newest review will be returned if the newest review points at the given commit. +// The returned PR Review will be nil if the user has not yet reviewed this PR. +func getNewestReviewStateApartFrom(ctx context.Context, userID, pullID int64, commitSHA string) (*ReviewState, error) { + var reviews []ReviewState + err := db.GetEngine(ctx).Where("user_id = ?", userID).And("pull_id = ?", pullID).OrderBy("updated_unix DESC").Limit(2).Find(&reviews) + // It would also be possible to use ".And("commit_sha != ?", commitSHA)" instead of the error handling below + // However, benchmarks show drastically improved performance by not doing that + + // Error cases in which no review should be returned + if err != nil || len(reviews) == 0 || (len(reviews) == 1 && reviews[0].CommitSHA == commitSHA) { + return nil, err + + // The first review points at the commit to exclude, hence skip to the second review + } else if len(reviews) >= 2 && reviews[0].CommitSHA == commitSHA { + return &reviews[1], nil + } + + // As we have no error cases left, the result must be the first element in the list + return &reviews[0], nil +} diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index f6b4f7764538..4b0cc8536b67 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -286,6 +286,15 @@ func (repo *Repository) GetPatch(base, head string, w io.Writer) error { return err } +// GetFilesChangedBetween returns a list of all files that have been changed between the given commits +func (repo *Repository) GetFilesChangedBetween(base, head string) ([]string, error) { + stdout, _, err := NewCommand(repo.Ctx, "diff", "--name-only", base+".."+head).RunStdString(&RunOpts{Dir: repo.Path}) + if err != nil { + return nil, err + } + return strings.Split(stdout, "\n"), err +} + // GetDiffFromMergeBase generates and return patch data from merge base to head func (repo *Repository) GetDiffFromMergeBase(base, head string, w io.Writer) error { stderr := new(bytes.Buffer) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 271fa62953bc..8c7269703fe9 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1493,6 +1493,9 @@ pulls.allow_edits_from_maintainers = Allow edits from maintainers pulls.allow_edits_from_maintainers_desc = Users with write access to the base branch can also push to this branch pulls.allow_edits_from_maintainers_err = Updating failed pulls.compare_changes_desc = Select the branch to merge into and the branch to pull from. +pulls.has_viewed_file = Viewed +pulls.has_changed_since_last_review = Changed since your last review +pulls.viewed_files_label = %[1]d / %[2]d files viewed pulls.compare_base = merge into pulls.compare_compare = pull from pulls.switch_comparison_type = Switch comparison type diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 7cedeec10e82..a0b356773857 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -685,22 +685,35 @@ func ViewPullFiles(ctx *context.Context) { if fileOnly && (len(files) == 2 || len(files) == 1) { maxLines, maxFiles = -1, -1 } + diffOptions := &gitdiff.DiffOptions{ + BeforeCommitID: startCommitID, + AfterCommitID: endCommitID, + SkipTo: ctx.FormString("skip-to"), + MaxLines: maxLines, + MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters, + MaxFiles: maxFiles, + WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)), + } - diff, err := gitdiff.GetDiff(gitRepo, - &gitdiff.DiffOptions{ - BeforeCommitID: startCommitID, - AfterCommitID: endCommitID, - SkipTo: ctx.FormString("skip-to"), - MaxLines: maxLines, - MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters, - MaxFiles: maxFiles, - WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)), - }, ctx.FormStrings("files")...) + var methodWithError string + var diff *gitdiff.Diff + if !ctx.IsSigned { + diff, err = gitdiff.GetDiff(gitRepo, diffOptions, files...) + methodWithError = "GetDiff" + } else { + diff, err = gitdiff.SyncAndGetUserSpecificDiff(ctx, ctx.Doer.ID, pull, gitRepo, diffOptions, files...) + methodWithError = "SyncAndGetUserSpecificDiff" + } if err != nil { - ctx.ServerError("GetDiffRangeWithWhitespaceBehavior", err) + ctx.ServerError(methodWithError, err) return } + ctx.PageData["prReview"] = map[string]interface{}{ + "numberOfFiles": diff.NumFiles, + "numberOfViewedFiles": diff.NumViewedFiles, + } + if err = diff.LoadComments(ctx, issue, ctx.Doer); err != nil { ctx.ServerError("LoadComments", err) return diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 939b0037a09e..98272ed48d8f 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -9,8 +9,10 @@ import ( "net/http" "code.gitea.io/gitea/models" + pull_model "code.gitea.io/gitea/models/pull" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/web" @@ -242,3 +244,47 @@ func DismissReview(ctx *context.Context) { ctx.Redirect(fmt.Sprintf("%s/pulls/%d#%s", ctx.Repo.RepoLink, comm.Issue.Index, comm.HashTag())) } + +// viewedFilesUpdate Struct to parse the body of a request to update the reviewed files of a PR +// If you want to implement an API to update the review, simply move this struct into modules. +type viewedFilesUpdate struct { + Files map[string]bool `json:"files"` + HeadCommitSHA string `json:"headCommitSHA"` +} + +func UpdateViewedFiles(ctx *context.Context) { + // Find corresponding PR + issue := checkPullInfo(ctx) + if ctx.Written() { + return + } + pull := issue.PullRequest + + var data *viewedFilesUpdate + err := json.NewDecoder(ctx.Req.Body).Decode(&data) + if err != nil { + log.Warn("Attempted to update a review but could not parse request body: %v", err) + ctx.Resp.WriteHeader(http.StatusBadRequest) + return + } + + // Expect the review to have been now if no head commit was supplied + if data.HeadCommitSHA == "" { + data.HeadCommitSHA = pull.HeadCommitID + } + + updatedFiles := make(map[string]pull_model.ViewedState, len(data.Files)) + for file, viewed := range data.Files { + + // Only unviewed and viewed are possible, has-changed can not be set from the outside + state := pull_model.Unviewed + if viewed { + state = pull_model.Viewed + } + updatedFiles[file] = state + } + + if err := pull_model.UpdateReviewState(ctx, ctx.Doer.ID, pull.ID, data.HeadCommitSHA, updatedFiles); err != nil { + ctx.ServerError("UpdateReview", err) + } +} diff --git a/routers/web/web.go b/routers/web/web.go index dcaad3d2bde1..38754025eee8 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -849,6 +849,7 @@ func RegisterRoutes(m *web.Route) { m.Post("/deadline", bindIgnErr(structs.EditDeadlineOption{}), repo.UpdateIssueDeadline) m.Post("/watch", repo.IssueWatch) m.Post("/ref", repo.UpdateIssueRef) + m.Post("/viewed-files", repo.UpdateViewedFiles) m.Group("/dependency", func() { m.Post("/add", repo.AddDependency) m.Post("/delete", repo.RemoveDependency) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 1df16e50167c..7fe056a4818b 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -22,6 +22,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/db" + pull_model "code.gitea.io/gitea/models/pull" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/analyze" "code.gitea.io/gitea/modules/charset" @@ -602,25 +603,27 @@ func (diffSection *DiffSection) GetComputedInlineDiffFor(diffLine *DiffLine) Dif // DiffFile represents a file diff. type DiffFile struct { - Name string - OldName string - Index int - Addition, Deletion int - Type DiffFileType - IsCreated bool - IsDeleted bool - IsBin bool - IsLFSFile bool - IsRenamed bool - IsAmbiguous bool - IsSubmodule bool - Sections []*DiffSection - IsIncomplete bool - IsIncompleteLineTooLong bool - IsProtected bool - IsGenerated bool - IsVendored bool - Language string + Name string + OldName string + Index int + Addition, Deletion int + Type DiffFileType + IsCreated bool + IsDeleted bool + IsBin bool + IsLFSFile bool + IsRenamed bool + IsAmbiguous bool + IsSubmodule bool + Sections []*DiffSection + IsIncomplete bool + IsIncompleteLineTooLong bool + IsProtected bool + IsGenerated bool + IsVendored bool + IsViewed bool // User specific + HasChangedSinceLastReview bool // User specific + Language string } // GetType returns type of diff file. @@ -663,6 +666,18 @@ func (diffFile *DiffFile) GetTailSection(gitRepo *git.Repository, leftCommitID, return tailSection } +// GetDiffFileName returns the name of the diff file, or its old name in case it was deleted +func (diffFile *DiffFile) GetDiffFileName() string { + if diffFile.Name == "" { + return diffFile.OldName + } + return diffFile.Name +} + +func (diffFile *DiffFile) ShouldBeHidden() bool { + return diffFile.IsGenerated || diffFile.IsViewed +} + func getCommitFileLineCount(commit *git.Commit, filePath string) int { blob, err := commit.GetBlobByPath(filePath) if err != nil { @@ -677,10 +692,12 @@ func getCommitFileLineCount(commit *git.Commit, filePath string) int { // Diff represents a difference between two git trees. type Diff struct { - Start, End string - NumFiles, TotalAddition, TotalDeletion int - Files []*DiffFile - IsIncomplete bool + Start, End string + NumFiles int + TotalAddition, TotalDeletion int + Files []*DiffFile + IsIncomplete bool + NumViewedFiles int // user-specific } // LoadComments loads comments into each line @@ -1497,6 +1514,70 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff return diff, nil } +// SyncAndGetUserSpecificDiff is like GetDiff, except that user specific data such as which files the given user has already viewed on the given PR will also be set +// Additionally, the database asynchronously is updated if files have changed since the last review +func SyncAndGetUserSpecificDiff(ctx context.Context, userID int64, pull *models.PullRequest, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) { + diff, err := GetDiff(gitRepo, opts, files...) + if err != nil { + return nil, err + } + review, err := pull_model.GetNewestReviewState(ctx, userID, pull.ID) + if err != nil || review == nil || review.UpdatedFiles == nil { + return diff, err + } + + latestCommit := opts.AfterCommitID + if latestCommit == "" { + latestCommit = pull.HeadBranch // opts.AfterCommitID is preferred because it handles PRs from forks correctly and the branch name doesn't + } + + changedFiles, err := gitRepo.GetFilesChangedBetween(review.CommitSHA, latestCommit) + if err != nil { + return diff, err + } + + filesChangedSinceLastDiff := make(map[string]pull_model.ViewedState) +outer: + for _, diffFile := range diff.Files { + fileViewedState := review.UpdatedFiles[diffFile.GetDiffFileName()] + + // Check whether it was previously detected that the file has changed since the last review + if fileViewedState == pull_model.HasChanged { + diffFile.HasChangedSinceLastReview = true + continue + } + + filename := diffFile.GetDiffFileName() + + // Check explicitly whether the file has changed since the last review + for _, changedFile := range changedFiles { + diffFile.HasChangedSinceLastReview = filename == changedFile + if diffFile.HasChangedSinceLastReview { + filesChangedSinceLastDiff[filename] = pull_model.HasChanged + continue outer // We don't want to check if the file is viewed here as that would fold the file, which is in this case unwanted + } + } + // Check whether the file has already been viewed + if fileViewedState == pull_model.Viewed { + diffFile.IsViewed = true + diff.NumViewedFiles++ + } + } + + // Explicitly store files that have changed in the database, if any is present at all. + // This has the benefit that the "Has Changed" attribute will be present as long as the user does not explicitly mark this file as viewed, so it will even survive a page reload after marking another file as viewed. + // On the other hand, this means that even if a commit reverting an unseen change is committed, the file will still be seen as changed. + if len(filesChangedSinceLastDiff) > 0 { + err := pull_model.UpdateReviewState(ctx, review.UserID, review.PullID, review.CommitSHA, filesChangedSinceLastDiff) + if err != nil { + log.Warn("Could not update review for user %d, pull %d, commit %s and the changed files %v: %v", review.UserID, review.PullID, review.CommitSHA, filesChangedSinceLastDiff, err) + return nil, err + } + } + + return diff, err +} + // CommentAsDiff returns c.Patch as *Diff func CommentAsDiff(c *models.Comment) (*Diff, error) { diff, err := ParsePatch(setting.Git.MaxGitDiffLines, diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index 6f67f36422ec..d6ad169821c2 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -18,6 +18,12 @@ {{svg "octicon-diff" 16 "mr-2"}}{{.i18n.Tr "repo.diff.stats_desc" .Diff.NumFiles .Diff.TotalAddition .Diff.TotalDeletion | Str2html}}
+ {{if and .PageIsPullFiles $.SignedUserID (not .IsArchived)}} + + + {{end}} {{template "repo/diff/whitespace_dropdown" .}} {{template "repo/diff/options_dropdown" .}} {{if and .PageIsPullFiles $.SignedUserID (not .IsArchived)}} @@ -58,11 +64,11 @@ {{$isCsv := (call $.IsCsvFile $file)}} {{$showFileViewToggle := or $isImage (and (not $file.IsIncomplete) $isCsv)}} {{$nameHash := Sha1 $file.Name}} -
+

- {{if $file.IsGenerated}} + {{if $file.ShouldBeHidden}} {{svg "octicon-chevron-right" 18}} {{else}} {{svg "octicon-chevron-down" 18}} @@ -106,9 +112,18 @@ {{$.i18n.Tr "repo.diff.view_file"}} {{end}} {{end}} + {{if and $.IsSigned $.PageIsPullFiles (not $.IsArchived)}} + {{if $file.HasChangedSinceLastReview}} + {{$.i18n.Tr "repo.pulls.has_changed_since_last_review"}} + {{end}} +
+ + +
+ {{end}}

-
+
{{if or $file.IsIncomplete $file.IsBin}}
diff --git a/web_src/js/features/file-fold.js b/web_src/js/features/file-fold.js new file mode 100644 index 000000000000..5e714a1de8da --- /dev/null +++ b/web_src/js/features/file-fold.js @@ -0,0 +1,18 @@ +import {svg} from '../svg.js'; + + +// Hides the file if newFold is true, and shows it otherwise. The actual hiding is performed using CSS. +// +// The fold arrow is the icon displayed on the upper left of the file box, especially intended for components having the 'fold-file' class. +// The file content box is the box that should be hidden or shown, especially intended for components having the 'file-content' class. +// +export function setFileFolding(fileContentBox, foldArrow, newFold) { + foldArrow.innerHTML = svg(`octicon-chevron-${newFold ? 'right' : 'down'}`, 18); + fileContentBox.setAttribute('data-folded', newFold); +} + +// Like `setFileFolding`, except that it automatically inverts the current file folding state. +export function invertFileFolding(fileContentBox, foldArrow) { + setFileFolding(fileContentBox, foldArrow, fileContentBox.getAttribute('data-folded') !== 'true'); +} + diff --git a/web_src/js/features/pull-view-file.js b/web_src/js/features/pull-view-file.js new file mode 100644 index 000000000000..57e9d5e0471a --- /dev/null +++ b/web_src/js/features/pull-view-file.js @@ -0,0 +1,71 @@ +import {setFileFolding} from './file-fold.js'; + +const {csrfToken, pageData} = window.config; +const prReview = pageData.prReview || {}; +const viewedStyleClass = 'viewed-file-checked-form'; +const viewedCheckboxSelector = '.viewed-file-form'; // Selector under which all "Viewed" checkbox forms can be found + + +// Refreshes the summary of viewed files if present +// The data used will be window.config.pageData.prReview.numberOf{Viewed}Files +function refreshViewedFilesSummary() { + const viewedFilesMeter = document.getElementById('viewed-files-summary'); + viewedFilesMeter?.setAttribute('value', prReview.numberOfViewedFiles); + const summaryLabel = document.getElementById('viewed-files-summary-label'); + if (summaryLabel) summaryLabel.innerHTML = summaryLabel.getAttribute('data-text-changed-template') + .replace('%[1]d', prReview.numberOfViewedFiles) + .replace('%[2]d', prReview.numberOfFiles); +} + +// Explicitly recounts how many files the user has currently reviewed by counting the number of checked "viewed" checkboxes +// Additionally, the viewed files summary will be updated if it exists +export function countAndUpdateViewedFiles() { + // The number of files is constant, but the number of viewed files can change because files can be loaded dynamically + prReview.numberOfViewedFiles = document.querySelectorAll(`${viewedCheckboxSelector} > input[type=checkbox][checked]`).length; + refreshViewedFilesSummary(); +} + +// Initializes a listener for all children of the given html element +// (for example 'document' in the most basic case) +// to watch for changes of viewed-file checkboxes +export function initViewedCheckboxListenerFor() { + for (const form of document.querySelectorAll(`${viewedCheckboxSelector}:not([data-has-viewed-checkbox-listener="true"])`)) { + // To prevent double addition of listeners + form.setAttribute('data-has-viewed-checkbox-listener', true); + + // The checkbox consists of a div containing the real checkbox with its label and the CSRF token, + // hence the actual checkbox first has to be found + const checkbox = form.querySelector('input[type=checkbox]'); + checkbox.addEventListener('change', function() { + // Mark the file as viewed visually - will especially change the background + if (this.checked) { + form.classList.add(viewedStyleClass); + prReview.numberOfViewedFiles++; + } else { + form.classList.remove(viewedStyleClass); + prReview.numberOfViewedFiles--; + } + + // Update viewed-files summary and remove "has changed" label if present + refreshViewedFilesSummary(); + const hasChangedLabel = form.parentNode.querySelector('.changed-since-last-review'); + hasChangedLabel?.parentNode.removeChild(hasChangedLabel); + + // Unfortunately, actual forms cause too many problems, hence another approach is needed + const files = {}; + files[checkbox.getAttribute('name')] = this.checked; + const data = {files}; + const headCommitSHA = form.getAttribute('data-headcommit'); + if (headCommitSHA) data.headCommitSHA = headCommitSHA; + fetch(form.getAttribute('data-link'), { + method: 'POST', + headers: {'X-Csrf-Token': csrfToken}, + body: JSON.stringify(data), + }); + + // Fold the file accordingly + const parentBox = form.closest('.diff-file-header'); + setFileFolding(parentBox.closest('.file-content'), parentBox.querySelector('.fold-file'), this.checked); + }); + } +} diff --git a/web_src/js/features/repo-code.js b/web_src/js/features/repo-code.js index d7b4baac83da..8562ba00723b 100644 --- a/web_src/js/features/repo-code.js +++ b/web_src/js/features/repo-code.js @@ -1,5 +1,6 @@ import $ from 'jquery'; import {svg} from '../svg.js'; +import {invertFileFolding} from './file-fold.js'; function changeHash(hash) { if (window.history.pushState) { @@ -148,10 +149,7 @@ export function initRepoCodeView() { }).trigger('hashchange'); } $(document).on('click', '.fold-file', ({currentTarget}) => { - const box = currentTarget.closest('.file-content'); - const folded = box.getAttribute('data-folded') !== 'true'; - currentTarget.innerHTML = svg(`octicon-chevron-${folded ? 'right' : 'down'}`, 18); - box.setAttribute('data-folded', String(folded)); + invertFileFolding(currentTarget.closest('.file-content'), currentTarget); }); $(document).on('click', '.blob-excerpt', async ({currentTarget}) => { const url = currentTarget.getAttribute('data-url'); diff --git a/web_src/js/features/repo-diff.js b/web_src/js/features/repo-diff.js index 013b8d3fa06e..8403a2fd42d1 100644 --- a/web_src/js/features/repo-diff.js +++ b/web_src/js/features/repo-diff.js @@ -2,6 +2,7 @@ import $ from 'jquery'; import {initCompReactionSelector} from './comp/ReactionSelector.js'; import {initRepoIssueContentHistory} from './repo-issue-content.js'; import {validateTextareaNonEmpty} from './comp/EasyMDE.js'; +import {initViewedCheckboxListenerFor, countAndUpdateViewedFiles} from './pull-view-file.js'; const {csrfToken} = window.config; @@ -104,6 +105,13 @@ export function initRepoDiffConversationNav() { }); } +// Will be called when the show more (files) button has been pressed +function onShowMoreFiles() { + initRepoIssueContentHistory(); + initViewedCheckboxListenerFor(); + countAndUpdateViewedFiles(); +} + export function initRepoDiffShowMore() { $('#diff-files, #diff-file-boxes').on('click', '#diff-show-more-files, #diff-show-more-files-stats', (e) => { e.preventDefault(); @@ -125,7 +133,7 @@ export function initRepoDiffShowMore() { $('#diff-too-many-files-stats').remove(); $('#diff-files').append($(resp).find('#diff-files li')); $('#diff-incomplete').replaceWith($(resp).find('#diff-file-boxes').children()); - initRepoIssueContentHistory(); + onShowMoreFiles(); }).fail(() => { $('#diff-show-more-files, #diff-show-more-files-stats').removeClass('disabled'); }); @@ -151,7 +159,7 @@ export function initRepoDiffShowMore() { } $target.parent().replaceWith($(resp).find('#diff-file-boxes .diff-file-body .file-body').children()); - initRepoIssueContentHistory(); + onShowMoreFiles(); }).fail(() => { $target.removeClass('disabled'); }); diff --git a/web_src/js/index.js b/web_src/js/index.js index 5b95a0d8efa8..4343c2d9652d 100644 --- a/web_src/js/index.js +++ b/web_src/js/index.js @@ -70,6 +70,7 @@ import { initRepoSettingsCollaboration, initRepoSettingSearchTeamBox, } from './features/repo-settings.js'; +import {initViewedCheckboxListenerFor} from './features/pull-view-file.js'; import {initOrgTeamSearchRepoBox, initOrgTeamSettings} from './features/org-team.js'; import {initUserAuthWebAuthn, initUserAuthWebAuthnRegister} from './features/user-auth-webauthn.js'; import {initRepoRelease, initRepoReleaseEditor} from './features/repo-release.js'; @@ -178,6 +179,6 @@ $(document).ready(() => { initUserAuthWebAuthn(); initUserAuthWebAuthnRegister(); initUserSettings(); - + initViewedCheckboxListenerFor(); checkAppUrl(); }); diff --git a/web_src/less/_review.less b/web_src/less/_review.less index 8bc41b973628..1bcb3663c26b 100644 --- a/web_src/less/_review.less +++ b/web_src/less/_review.less @@ -262,3 +262,21 @@ a.blob-excerpt:hover { scroll-margin-top: 130px; } } + +.changed-since-last-review { + margin: 0 5px; + padding: 0 3px; + border: 2px var(--color-primary-light-3) solid; + background-color: var(--color-primary-alpha-30); + border-radius: 7px; +} + +.viewed-file-form { + margin: 0 3px; + padding: 0 3px; + border-radius: 3px; +} + +.viewed-file-checked-form { + background-color: var(--color-primary-light-4); +}