From 7ad50313284db7eec565ad1750108de1444c5a84 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Tue, 30 Apr 2024 11:53:16 +0800 Subject: [PATCH 1/6] Fix duplicate status check contexts (#30660) Caused by #30076. There may be some duplicate status check contexts when setting status checks for a branch protection rule. The duplicate contexts should be removed. Before: After: --- models/git/commit_status.go | 30 +++-------------- models/git/commit_status_test.go | 56 ++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 25 deletions(-) diff --git a/models/git/commit_status.go b/models/git/commit_status.go index c3cda7b73d..d12afc42c5 100644 --- a/models/git/commit_status.go +++ b/models/git/commit_status.go @@ -397,36 +397,16 @@ func GetLatestCommitStatusForRepoCommitIDs(ctx context.Context, repoID int64, co // FindRepoRecentCommitStatusContexts returns repository's recent commit status contexts func FindRepoRecentCommitStatusContexts(ctx context.Context, repoID int64, before time.Duration) ([]string, error) { - type result struct { - Index int64 - SHA string - } - getBase := func() *xorm.Session { - return db.GetEngine(ctx).Table(&CommitStatus{}).Where("repo_id = ?", repoID) - } - start := timeutil.TimeStampNow().AddDuration(-before) - results := make([]result, 0, 10) - sess := getBase().And("updated_unix >= ?", start). - Select("max( `index` ) as `index`, sha"). - GroupBy("context_hash, sha").OrderBy("max( `index` ) desc") - - err := sess.Find(&results) - if err != nil { + var contexts []string + if err := db.GetEngine(ctx).Table("commit_status"). + Where("repo_id = ?", repoID).And("updated_unix >= ?", start). + Cols("context").Distinct().Find(&contexts); err != nil { return nil, err } - contexts := make([]string, 0, len(results)) - if len(results) == 0 { - return contexts, nil - } - - conds := make([]builder.Cond, 0, len(results)) - for _, result := range results { - conds = append(conds, builder.Eq{"`index`": result.Index, "sha": result.SHA}) - } - return contexts, getBase().And(builder.Or(conds...)).Select("context").Find(&contexts) + return contexts, nil } // NewCommitStatusOptions holds options for creating a CommitStatus diff --git a/models/git/commit_status_test.go b/models/git/commit_status_test.go index 74ba4a1006..08eba6e293 100644 --- a/models/git/commit_status_test.go +++ b/models/git/commit_status_test.go @@ -5,11 +5,15 @@ package git_test import ( "testing" + "time" "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/structs" "github.com/stretchr/testify/assert" @@ -175,3 +179,55 @@ func Test_CalcCommitStatus(t *testing.T) { assert.Equal(t, kase.expected, git_model.CalcCommitStatus(kase.statuses)) } } + +func TestFindRepoRecentCommitStatusContexts(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + gitRepo, err := gitrepo.OpenRepository(git.DefaultContext, repo2) + assert.NoError(t, err) + defer gitRepo.Close() + + commit, err := gitRepo.GetBranchCommit(repo2.DefaultBranch) + assert.NoError(t, err) + + defer func() { + _, err := db.DeleteByBean(db.DefaultContext, &git_model.CommitStatus{ + RepoID: repo2.ID, + CreatorID: user2.ID, + SHA: commit.ID.String(), + }) + assert.NoError(t, err) + }() + + err = git_model.NewCommitStatus(db.DefaultContext, git_model.NewCommitStatusOptions{ + Repo: repo2, + Creator: user2, + SHA: commit.ID, + CommitStatus: &git_model.CommitStatus{ + State: structs.CommitStatusFailure, + TargetURL: "https://example.com/tests/", + Context: "compliance/lint-backend", + }, + }) + assert.NoError(t, err) + + err = git_model.NewCommitStatus(db.DefaultContext, git_model.NewCommitStatusOptions{ + Repo: repo2, + Creator: user2, + SHA: commit.ID, + CommitStatus: &git_model.CommitStatus{ + State: structs.CommitStatusSuccess, + TargetURL: "https://example.com/tests/", + Context: "compliance/lint-backend", + }, + }) + assert.NoError(t, err) + + contexts, err := git_model.FindRepoRecentCommitStatusContexts(db.DefaultContext, repo2.ID, time.Hour) + assert.NoError(t, err) + if assert.Len(t, contexts, 1) { + assert.Equal(t, "compliance/lint-backend", contexts[0]) + } +} From 059b2718a5615c01b897283f6ae53c9702f11239 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 30 Apr 2024 12:26:13 +0800 Subject: [PATCH 2/6] Right align the "Settings" menu item in overflow-menu (#30764) I guess there could be enough people liking to make the Settings menu item right aligned. As a site admin, I found it's easier to find the right-aligned Settings menu item. Tested with various sizes: ![image](https://github.com/go-gitea/gitea/assets/2114189/92836527-2cb2-4531-9296-233c5bd698f4) ![image](https://github.com/go-gitea/gitea/assets/2114189/3a0729fc-5e33-44b5-9fb4-3a4e787405b5) ![image](https://github.com/go-gitea/gitea/assets/2114189/9845ab6b-88e3-4e5a-8d6d-2b8af259d593) --- templates/org/menu.tmpl | 3 ++- templates/repo/header.tmpl | 1 + web_src/css/base.css | 17 ++++++++++++ web_src/css/modules/container.css | 32 ----------------------- web_src/js/webcomponents/overflow-menu.js | 24 +++++++++++++---- 5 files changed, 39 insertions(+), 38 deletions(-) diff --git a/templates/org/menu.tmpl b/templates/org/menu.tmpl index c519606d1f..698a9559c5 100644 --- a/templates/org/menu.tmpl +++ b/templates/org/menu.tmpl @@ -40,8 +40,9 @@ {{end}} {{if .IsOrganizationOwner}} + - {{svg "octicon-tools"}} {{ctx.Locale.Tr "repo.settings"}} + {{svg "octicon-tools"}} {{ctx.Locale.Tr "repo.settings"}} {{end}} diff --git a/templates/repo/header.tmpl b/templates/repo/header.tmpl index 775aa30063..c0d833a187 100644 --- a/templates/repo/header.tmpl +++ b/templates/repo/header.tmpl @@ -216,6 +216,7 @@ {{template "custom/extra_tabs" .}} {{if .Permission.IsAdmin}} + {{svg "octicon-tools"}} {{ctx.Locale.Tr "repo.settings"}} diff --git a/web_src/css/base.css b/web_src/css/base.css index df9028b50a..1d65bb37e7 100644 --- a/web_src/css/base.css +++ b/web_src/css/base.css @@ -938,6 +938,23 @@ overflow-menu .overflow-menu-items .item { margin-bottom: 0 !important; /* reset fomantic's margin, because the active menu has special bottom border */ } +overflow-menu .overflow-menu-items .item-flex-space { + flex: 1; +} + +overflow-menu .overflow-menu-button { + background: transparent; + border: none; + color: inherit; + text-align: center; + width: 32px; + padding: 0; +} + +overflow-menu .overflow-menu-button:hover { + color: var(--color-text-dark); +} + overflow-menu .ui.label { margin-left: 7px !important; /* save some space */ } diff --git a/web_src/css/modules/container.css b/web_src/css/modules/container.css index f394d6c06d..9f67ceb8d5 100644 --- a/web_src/css/modules/container.css +++ b/web_src/css/modules/container.css @@ -6,38 +6,6 @@ max-width: 100%; } -@media (max-width: 767.98px) { - .ui.ui.ui.container:not(.fluid) { - width: auto; - margin-left: 1em; - margin-right: 1em; - } -} - -@media (min-width: 768px) and (max-width: 991.98px) { - .ui.ui.ui.container:not(.fluid) { - width: 723px; - margin-left: auto; - margin-right: auto; - } -} - -@media (min-width: 992px) and (max-width: 1199.98px) { - .ui.ui.ui.container:not(.fluid) { - width: 933px; - margin-left: auto; - margin-right: auto; - } -} - -@media (min-width: 1200px) { - .ui.ui.ui.container:not(.fluid) { - width: 1127px; - margin-left: auto; - margin-right: auto; - } -} - .ui.fluid.container { width: 100%; } diff --git a/web_src/js/webcomponents/overflow-menu.js b/web_src/js/webcomponents/overflow-menu.js index 604fce7d4b..0778c5990f 100644 --- a/web_src/js/webcomponents/overflow-menu.js +++ b/web_src/js/webcomponents/overflow-menu.js @@ -8,7 +8,7 @@ window.customElements.define('overflow-menu', class extends HTMLElement { if (!this.tippyContent) { const div = document.createElement('div'); div.classList.add('tippy-target'); - div.tabIndex = '-1'; // for initial focus, programmatic focus only + div.tabIndex = -1; // for initial focus, programmatic focus only div.addEventListener('keydown', (e) => { if (e.key === 'Tab') { const items = this.tippyContent.querySelectorAll('[role="menuitem"]'); @@ -60,21 +60,35 @@ window.customElements.define('overflow-menu', class extends HTMLElement { this.tippyContent = div; } + const itemFlexSpace = this.menuItemsEl.querySelector('.item-flex-space'); + // move items in tippy back into the menu items for subsequent measurement for (const item of this.tippyItems || []) { - this.menuItemsEl.append(item); + if (!itemFlexSpace || item.getAttribute('data-after-flex-space')) { + this.menuItemsEl.append(item); + } else { + itemFlexSpace.insertAdjacentElement('beforebegin', item); + } } // measure which items are partially outside the element and move them into the button menu + itemFlexSpace?.style.setProperty('display', 'none', 'important'); this.tippyItems = []; const menuRight = this.offsetLeft + this.offsetWidth; - const menuItems = this.menuItemsEl.querySelectorAll('.item'); + const menuItems = this.menuItemsEl.querySelectorAll('.item, .item-flex-space'); + let afterFlexSpace = false; for (const item of menuItems) { + if (item.classList.contains('item-flex-space')) { + afterFlexSpace = true; + continue; + } + if (afterFlexSpace) item.setAttribute('data-after-flex-space', 'true'); const itemRight = item.offsetLeft + item.offsetWidth; - if (menuRight - itemRight < 38) { // roughly the width of .overflow-menu-button + if (menuRight - itemRight < 38) { // roughly the width of .overflow-menu-button with some extra space this.tippyItems.push(item); } } + itemFlexSpace?.style.removeProperty('display'); // if there are no overflown items, remove any previously created button if (!this.tippyItems?.length) { @@ -105,7 +119,7 @@ window.customElements.define('overflow-menu', class extends HTMLElement { // create button initially const btn = document.createElement('button'); - btn.classList.add('overflow-menu-button', 'btn', 'tw-px-2', 'hover:tw-text-text-dark'); + btn.classList.add('overflow-menu-button'); btn.setAttribute('aria-label', window.config.i18n.more_items); btn.innerHTML = octiconKebabHorizontal; this.append(btn); From f2d8ccc5bb2df25557cc0d4d23f2cdd029358274 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 30 Apr 2024 10:43:08 +0200 Subject: [PATCH 3/6] Get repo assignees and reviewers should ignore deactivated users (#30770) If an user is deactivated, it should not be in the list of users who are suggested to be assigned or review-requested. old assignees or reviewers are not affected. --- *Sponsored by Kithara Software GmbH* --- models/repo/user_repo.go | 8 ++++++-- models/repo/user_repo_test.go | 22 +++++++++++++++++----- tests/integration/api_repo_test.go | 4 +++- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/models/repo/user_repo.go b/models/repo/user_repo.go index 1c5412fe7d..c305603e02 100644 --- a/models/repo/user_repo.go +++ b/models/repo/user_repo.go @@ -130,7 +130,10 @@ func GetRepoAssignees(ctx context.Context, repo *Repository) (_ []*user_model.Us // and just waste 1 unit is cheaper than re-allocate memory once. users := make([]*user_model.User, 0, len(uniqueUserIDs)+1) if len(userIDs) > 0 { - if err = e.In("id", uniqueUserIDs.Values()).OrderBy(user_model.GetOrderByName()).Find(&users); err != nil { + if err = e.In("id", uniqueUserIDs.Values()). + Where(builder.Eq{"`user`.is_active": true}). + OrderBy(user_model.GetOrderByName()). + Find(&users); err != nil { return nil, err } } @@ -152,7 +155,8 @@ func GetReviewers(ctx context.Context, repo *Repository, doerID, posterID int64) return nil, err } - cond := builder.And(builder.Neq{"`user`.id": posterID}) + cond := builder.And(builder.Neq{"`user`.id": posterID}). + And(builder.Eq{"`user`.is_active": true}) if repo.IsPrivate || repo.Owner.Visibility == api.VisibleTypePrivate { // This a private repository: diff --git a/models/repo/user_repo_test.go b/models/repo/user_repo_test.go index 591dcea5b5..d2bf6dc912 100644 --- a/models/repo/user_repo_test.go +++ b/models/repo/user_repo_test.go @@ -9,6 +9,7 @@ import ( "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" "github.com/stretchr/testify/assert" ) @@ -25,8 +26,17 @@ func TestRepoAssignees(t *testing.T) { repo21 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 21}) users, err = repo_model.GetRepoAssignees(db.DefaultContext, repo21) assert.NoError(t, err) - assert.Len(t, users, 4) - assert.ElementsMatch(t, []int64{10, 15, 16, 18}, []int64{users[0].ID, users[1].ID, users[2].ID, users[3].ID}) + if assert.Len(t, users, 4) { + assert.ElementsMatch(t, []int64{10, 15, 16, 18}, []int64{users[0].ID, users[1].ID, users[2].ID, users[3].ID}) + } + + // do not return deactivated users + assert.NoError(t, user_model.UpdateUserCols(db.DefaultContext, &user_model.User{ID: 15, IsActive: false}, "is_active")) + users, err = repo_model.GetRepoAssignees(db.DefaultContext, repo21) + assert.NoError(t, err) + if assert.Len(t, users, 3) { + assert.NotContains(t, []int64{users[0].ID, users[1].ID, users[2].ID}, 15) + } } func TestRepoGetReviewers(t *testing.T) { @@ -38,17 +48,19 @@ func TestRepoGetReviewers(t *testing.T) { ctx := db.DefaultContext reviewers, err := repo_model.GetReviewers(ctx, repo1, 2, 2) assert.NoError(t, err) - assert.Len(t, reviewers, 4) + if assert.Len(t, reviewers, 3) { + assert.ElementsMatch(t, []int64{1, 4, 11}, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID}) + } // should include doer if doer is not PR poster. reviewers, err = repo_model.GetReviewers(ctx, repo1, 11, 2) assert.NoError(t, err) - assert.Len(t, reviewers, 4) + assert.Len(t, reviewers, 3) // should not include PR poster, if PR poster would be otherwise eligible reviewers, err = repo_model.GetReviewers(ctx, repo1, 11, 4) assert.NoError(t, err) - assert.Len(t, reviewers, 3) + assert.Len(t, reviewers, 2) // test private user repo repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go index bc2720d51e..f33827e58b 100644 --- a/tests/integration/api_repo_test.go +++ b/tests/integration/api_repo_test.go @@ -684,7 +684,9 @@ func TestAPIRepoGetReviewers(t *testing.T) { resp := MakeRequest(t, req, http.StatusOK) var reviewers []*api.User DecodeJSON(t, resp, &reviewers) - assert.Len(t, reviewers, 4) + if assert.Len(t, reviewers, 3) { + assert.ElementsMatch(t, []int64{1, 4, 11}, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID}) + } } func TestAPIRepoGetAssignees(t *testing.T) { From 610802df85933e7a190a705bc3f7800da87ce868 Mon Sep 17 00:00:00 2001 From: silverwind Date: Tue, 30 Apr 2024 14:34:40 +0200 Subject: [PATCH 4/6] Fix tautological conditions (#30735) As discovered by https://github.com/go-gitea/gitea/pull/30729. --------- Co-authored-by: Giteabot --- modules/indexer/code/indexer.go | 6 ------ routers/private/hook_post_receive.go | 18 ++++++++---------- services/auth/source/oauth2/providers.go | 2 +- services/convert/issue.go | 14 ++++++++------ tests/integration/git_test.go | 9 +++------ 5 files changed, 20 insertions(+), 29 deletions(-) diff --git a/modules/indexer/code/indexer.go b/modules/indexer/code/indexer.go index ebebf6ba8a..c1ab26569c 100644 --- a/modules/indexer/code/indexer.go +++ b/modules/indexer/code/indexer.go @@ -178,12 +178,6 @@ func Init() { }() rIndexer = elasticsearch.NewIndexer(setting.Indexer.RepoConnStr, setting.Indexer.RepoIndexerName) - if err != nil { - cancel() - (*globalIndexer.Load()).Close() - close(waitChannel) - log.Fatal("PID: %d Unable to create the elasticsearch Repository Indexer connstr: %s Error: %v", os.Getpid(), setting.Indexer.RepoConnStr, err) - } existed, err = rIndexer.Init(ctx) if err != nil { cancel() diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 769a68970d..adc435b42c 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -117,16 +117,14 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } } if len(branchesToSync) > 0 { - if gitRepo == nil { - var err error - gitRepo, err = gitrepo.OpenRepository(ctx, repo) - if err != nil { - log.Error("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err), - }) - return - } + var err error + gitRepo, err = gitrepo.OpenRepository(ctx, repo) + if err != nil { + log.Error("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err), + }) + return } var ( diff --git a/services/auth/source/oauth2/providers.go b/services/auth/source/oauth2/providers.go index 6ed6c184eb..f2c1bb4894 100644 --- a/services/auth/source/oauth2/providers.go +++ b/services/auth/source/oauth2/providers.go @@ -182,7 +182,7 @@ func createProvider(providerName string, source *Source) (goth.Provider, error) } // always set the name if provider is created so we can support multiple setups of 1 provider - if err == nil && provider != nil { + if provider != nil { provider.SetName(providerName) } diff --git a/services/convert/issue.go b/services/convert/issue.go index 54b00cd88e..668affe09a 100644 --- a/services/convert/issue.go +++ b/services/convert/issue.go @@ -211,13 +211,11 @@ func ToLabel(label *issues_model.Label, repo *repo_model.Repository, org *user_m IsArchived: label.IsArchived(), } + labelBelongsToRepo := label.BelongsToRepo() + // calculate URL - if label.BelongsToRepo() && repo != nil { - if repo != nil { - result.URL = fmt.Sprintf("%s/labels/%d", repo.APIURL(), label.ID) - } else { - log.Error("ToLabel did not get repo to calculate url for label with id '%d'", label.ID) - } + if labelBelongsToRepo && repo != nil { + result.URL = fmt.Sprintf("%s/labels/%d", repo.APIURL(), label.ID) } else { // BelongsToOrg if org != nil { result.URL = fmt.Sprintf("%sapi/v1/orgs/%s/labels/%d", setting.AppURL, url.PathEscape(org.Name), label.ID) @@ -226,6 +224,10 @@ func ToLabel(label *issues_model.Label, repo *repo_model.Repository, org *user_m } } + if labelBelongsToRepo && repo == nil { + log.Error("ToLabel did not get repo to calculate url for label with id '%d'", label.ID) + } + return result } diff --git a/tests/integration/git_test.go b/tests/integration/git_test.go index 74c511fd7e..8a091ecab7 100644 --- a/tests/integration/git_test.go +++ b/tests/integration/git_test.go @@ -81,7 +81,7 @@ func testGit(t *testing.T, u *url.URL) { rawTest(t, &httpContext, little, big, littleLFS, bigLFS) mediaTest(t, &httpContext, little, big, littleLFS, bigLFS) - t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &httpContext, "master", "test/head")) + t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &httpContext, "test/head")) t.Run("BranchProtectMerge", doBranchProtectPRMerge(&httpContext, dstPath)) t.Run("AutoMerge", doAutoPRMerge(&httpContext, dstPath)) t.Run("CreatePRAndSetManuallyMerged", doCreatePRAndSetManuallyMerged(httpContext, httpContext, dstPath, "master", "test-manually-merge")) @@ -122,7 +122,7 @@ func testGit(t *testing.T, u *url.URL) { rawTest(t, &sshContext, little, big, littleLFS, bigLFS) mediaTest(t, &sshContext, little, big, littleLFS, bigLFS) - t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &sshContext, "master", "test/head2")) + t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &sshContext, "test/head2")) t.Run("BranchProtectMerge", doBranchProtectPRMerge(&sshContext, dstPath)) t.Run("MergeFork", func(t *testing.T) { defer tests.PrintCurrentTest(t)() @@ -329,9 +329,6 @@ func generateCommitWithNewData(size int, repoPath, email, fullName, prefix strin } written += n } - if err != nil { - return "", err - } // Commit // Now here we should explicitly allow lfs filters to run @@ -693,7 +690,7 @@ func doAutoPRMerge(baseCtx *APITestContext, dstPath string) func(t *testing.T) { } } -func doCreateAgitFlowPull(dstPath string, ctx *APITestContext, baseBranch, headBranch string) func(t *testing.T) { +func doCreateAgitFlowPull(dstPath string, ctx *APITestContext, headBranch string) func(t *testing.T) { return func(t *testing.T) { defer tests.PrintCurrentTest(t)() From 5f05e7b41a57972cc418a125d9263173b7b9838f Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 30 Apr 2024 20:39:36 +0800 Subject: [PATCH 5/6] Fix dashboard commit status null access (#30771) Fix #30768 --- web_src/js/components/DashboardRepoList.vue | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/web_src/js/components/DashboardRepoList.vue b/web_src/js/components/DashboardRepoList.vue index 2d980a1b18..8bce40ee79 100644 --- a/web_src/js/components/DashboardRepoList.vue +++ b/web_src/js/components/DashboardRepoList.vue @@ -251,9 +251,9 @@ const sfc = { this.repos = json.data.map((webSearchRepo) => { return { ...webSearchRepo.repository, - latest_commit_status_state: webSearchRepo.latest_commit_status.State, + latest_commit_status_state: webSearchRepo.latest_commit_status?.State, // if latest_commit_status is null, it means there is no commit status + latest_commit_status_state_link: webSearchRepo.latest_commit_status?.TargetURL, locale_latest_commit_status_state: webSearchRepo.locale_latest_commit_status, - latest_commit_status_state_link: webSearchRepo.latest_commit_status.TargetURL, }; }); const count = response.headers.get('X-Total-Count'); From 564102ce89f53d6bd2fdbaa33416e4287d6fe9a8 Mon Sep 17 00:00:00 2001 From: silverwind Date: Tue, 30 Apr 2024 16:52:46 +0200 Subject: [PATCH 6/6] Rework and fix stopwatch (#30732) Fixes https://github.com/go-gitea/gitea/issues/30721 and overhauls the stopwatch. Time is now shown inside the "dot" icon and on both mobile and desktop. All rendering is now done by ``, the `pretty-ms` dependency is dropped. Desktop: Screenshot 2024-04-29 at 22 33 27 Mobile: Screenshot 2024-04-29 at 22 34 19 Note for tippy: Previously, tippy instances defaulted to "menu" theme, but that theme is really only meant for `.ui.menu`, so it was not optimal for the stopwatch popover. This introduces a unopinionated `default` theme that has no padding and should be suitable for all content. I reviewed all existing uses and explicitely set the desired `theme` on all of them. --- package-lock.json | 26 ------- package.json | 1 - templates/base/head_navbar.tmpl | 69 ++++++++++--------- web_src/css/modules/navbar.css | 16 ++--- web_src/css/modules/tippy.css | 7 +- web_src/js/features/contextpopup.js | 2 + web_src/js/features/repo-code.js | 1 + web_src/js/features/repo-issue.js | 1 + web_src/js/features/stopwatch.js | 82 +++++++++++------------ web_src/js/modules/tippy.js | 6 +- web_src/js/webcomponents/overflow-menu.js | 1 + 11 files changed, 99 insertions(+), 113 deletions(-) diff --git a/package-lock.json b/package-lock.json index 8e4eeb7fb8..917ff1029b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -42,7 +42,6 @@ "postcss": "8.4.38", "postcss-loader": "8.1.1", "postcss-nesting": "12.1.2", - "pretty-ms": "9.0.0", "sortablejs": "1.15.2", "swagger-ui-dist": "5.17.2", "tailwindcss": "3.4.3", @@ -9170,17 +9169,6 @@ "url": "https://github.com/sponsors/sindresorhus" } }, - "node_modules/parse-ms": { - "version": "4.0.0", - "resolved": "https://registry.npmjs.org/parse-ms/-/parse-ms-4.0.0.tgz", - "integrity": "sha512-TXfryirbmq34y8QBwgqCVLi+8oA3oWx2eAnSn62ITyEhEYaWRlVZ2DvMM9eZbMs/RfxPu/PK/aBLyGj4IrqMHw==", - "engines": { - "node": ">=18" - }, - "funding": { - "url": "https://github.com/sponsors/sindresorhus" - } - }, "node_modules/path-exists": { "version": "4.0.0", "resolved": "https://registry.npmjs.org/path-exists/-/path-exists-4.0.0.tgz", @@ -9772,20 +9760,6 @@ "url": "https://github.com/chalk/ansi-styles?sponsor=1" } }, - "node_modules/pretty-ms": { - "version": "9.0.0", - "resolved": "https://registry.npmjs.org/pretty-ms/-/pretty-ms-9.0.0.tgz", - "integrity": "sha512-E9e9HJ9R9NasGOgPaPE8VMeiPKAyWR5jcFpNnwIejslIhWqdqOrb2wShBsncMPUb+BcCd2OPYfh7p2W6oemTng==", - "dependencies": { - "parse-ms": "^4.0.0" - }, - "engines": { - "node": ">=18" - }, - "funding": { - "url": "https://github.com/sponsors/sindresorhus" - } - }, "node_modules/printable-characters": { "version": "1.0.42", "resolved": "https://registry.npmjs.org/printable-characters/-/printable-characters-1.0.42.tgz", diff --git a/package.json b/package.json index 142b9bb3ee..5f9b810320 100644 --- a/package.json +++ b/package.json @@ -41,7 +41,6 @@ "postcss": "8.4.38", "postcss-loader": "8.1.1", "postcss-nesting": "12.1.2", - "pretty-ms": "9.0.0", "sortablejs": "1.15.2", "swagger-ui-dist": "5.17.2", "tailwindcss": "3.4.3", diff --git a/templates/base/head_navbar.tmpl b/templates/base/head_navbar.tmpl index addff22c49..7a3e663c49 100644 --- a/templates/base/head_navbar.tmpl +++ b/templates/base/head_navbar.tmpl @@ -12,6 +12,14 @@
+ {{if and .IsSigned EnableTimetracking .ActiveStopwatch}} + +
+ {{svg "octicon-stopwatch"}} + +
+ + {{end}} {{if .IsSigned}}
@@ -74,41 +82,13 @@
{{else if .IsSigned}} - {{if EnableTimetracking}} - + {{if and EnableTimetracking .ActiveStopwatch}} +
{{svg "octicon-stopwatch"}}
- {{ctx.Locale.Tr "active_stopwatch"}} -
-
- - {{svg "octicon-issue-opened" 16 "tw-mr-2"}} - {{.ActiveStopwatch.RepoSlug}}#{{.ActiveStopwatch.IssueIndex}} - - {{if .ActiveStopwatch}}{{Sec2Time .ActiveStopwatch.Seconds}}{{end}} - - -
- {{.CsrfTokenHtml}} - -
-
- {{.CsrfTokenHtml}} - -
-
-
{{end}} @@ -202,4 +182,33 @@ {{end}} + + {{if and .IsSigned EnableTimetracking .ActiveStopwatch}} +
+
+ + {{svg "octicon-issue-opened" 16}} + {{.ActiveStopwatch.RepoSlug}}#{{.ActiveStopwatch.IssueIndex}} + +
+
+ {{.CsrfTokenHtml}} + +
+
+ {{.CsrfTokenHtml}} + +
+
+
+
+ {{end}} diff --git a/web_src/css/modules/navbar.css b/web_src/css/modules/navbar.css index d7aa197e02..848f9331d0 100644 --- a/web_src/css/modules/navbar.css +++ b/web_src/css/modules/navbar.css @@ -103,19 +103,12 @@ width: 50%; min-height: 48px; } + #navbar #mobile-stopwatch-icon, #navbar #mobile-notifications-icon { margin-right: 6px !important; } } -#navbar a.item .notification_count { - color: var(--color-nav-bg); - padding: 0 3.75px; - font-size: 12px; - line-height: 12px; - font-weight: var(--font-weight-bold); -} - #navbar a.item:hover .notification_count, #navbar a.item:hover .header-stopwatch-dot { border-color: var(--color-nav-hover-bg); @@ -123,6 +116,11 @@ #navbar a.item .notification_count, #navbar a.item .header-stopwatch-dot { + color: var(--color-nav-bg); + padding: 0 3.75px; + font-size: 12px; + line-height: 12px; + font-weight: var(--font-weight-bold); background: var(--color-primary); border: 2px solid var(--color-nav-bg); position: absolute; @@ -135,6 +133,8 @@ align-items: center; justify-content: center; z-index: 1; /* prevent menu button background from overlaying icon */ + user-select: none; + white-space: nowrap; } .secondary-nav { diff --git a/web_src/css/modules/tippy.css b/web_src/css/modules/tippy.css index 6ac7c37d93..53c3d5aaea 100644 --- a/web_src/css/modules/tippy.css +++ b/web_src/css/modules/tippy.css @@ -16,8 +16,8 @@ .tippy-box { position: relative; - background-color: var(--color-body); - color: var(--color-secondary-dark-6); + background-color: var(--color-menu); + color: var(--color-text); border: 1px solid var(--color-secondary); border-radius: var(--border-radius); font-size: 1rem; @@ -25,7 +25,6 @@ .tippy-content { position: relative; - padding: 1rem; /* if you need different padding, use different data-theme */ z-index: 1; } @@ -166,5 +165,5 @@ } .tippy-svg-arrow-inner { - fill: var(--color-body); + fill: var(--color-menu); } diff --git a/web_src/js/features/contextpopup.js b/web_src/js/features/contextpopup.js index ce90f3e505..6a9325ed1c 100644 --- a/web_src/js/features/contextpopup.js +++ b/web_src/js/features/contextpopup.js @@ -18,6 +18,7 @@ export function attachRefIssueContextPopup(refIssues) { if (!owner) return; const el = document.createElement('div'); + el.classList.add('tw-p-3'); refIssue.parentNode.insertBefore(el, refIssue.nextSibling); const view = createApp(ContextPopup); @@ -30,6 +31,7 @@ export function attachRefIssueContextPopup(refIssues) { } createTippy(refIssue, { + theme: 'default', content: el, placement: 'top-start', interactive: true, diff --git a/web_src/js/features/repo-code.js b/web_src/js/features/repo-code.js index 63da5f2039..7c74c253a2 100644 --- a/web_src/js/features/repo-code.js +++ b/web_src/js/features/repo-code.js @@ -113,6 +113,7 @@ function showLineButton() { btn.closest('.code-view').append(menu.cloneNode(true)); createTippy(btn, { + theme: 'menu', trigger: 'click', hideOnClick: true, content: menu, diff --git a/web_src/js/features/repo-issue.js b/web_src/js/features/repo-issue.js index 2b2eed58bb..c4e14c62c4 100644 --- a/web_src/js/features/repo-issue.js +++ b/web_src/js/features/repo-issue.js @@ -502,6 +502,7 @@ export function initRepoPullRequestReview() { if ($reviewBtn.length && $panel.length) { const tippy = createTippy($reviewBtn[0], { content: $panel[0], + theme: 'default', placement: 'bottom', trigger: 'click', maxWidth: 'none', diff --git a/web_src/js/features/stopwatch.js b/web_src/js/features/stopwatch.js index 2ec74344fc..bcea26bd6e 100644 --- a/web_src/js/features/stopwatch.js +++ b/web_src/js/features/stopwatch.js @@ -1,4 +1,3 @@ -import prettyMilliseconds from 'pretty-ms'; import {createTippy} from '../modules/tippy.js'; import {GET} from '../modules/fetch.js'; import {hideElem, showElem} from '../utils/dom.js'; @@ -10,28 +9,31 @@ export function initStopwatch() { return; } - const stopwatchEl = document.querySelector('.active-stopwatch-trigger'); + const stopwatchEls = document.querySelectorAll('.active-stopwatch'); const stopwatchPopup = document.querySelector('.active-stopwatch-popup'); - if (!stopwatchEl || !stopwatchPopup) { + if (!stopwatchEls.length || !stopwatchPopup) { return; } - stopwatchEl.removeAttribute('href'); // intended for noscript mode only - - createTippy(stopwatchEl, { - content: stopwatchPopup, - placement: 'bottom-end', - trigger: 'click', - maxWidth: 'none', - interactive: true, - hideOnClick: true, - }); - // global stop watch (in the head_navbar), it should always work in any case either the EventSource or the PeriodicPoller is used. - const currSeconds = document.querySelector('.stopwatch-time')?.getAttribute('data-seconds'); - if (currSeconds) { - updateStopwatchTime(currSeconds); + const seconds = stopwatchEls[0]?.getAttribute('data-seconds'); + if (seconds) { + updateStopwatchTime(parseInt(seconds)); + } + + for (const stopwatchEl of stopwatchEls) { + stopwatchEl.removeAttribute('href'); // intended for noscript mode only + + createTippy(stopwatchEl, { + content: stopwatchPopup.cloneNode(true), + placement: 'bottom-end', + trigger: 'click', + maxWidth: 'none', + interactive: true, + hideOnClick: true, + theme: 'default', + }); } let usingPeriodicPoller = false; @@ -124,10 +126,9 @@ async function updateStopwatch() { function updateStopwatchData(data) { const watch = data[0]; - const btnEl = document.querySelector('.active-stopwatch-trigger'); + const btnEls = document.querySelectorAll('.active-stopwatch'); if (!watch) { - clearStopwatchTimer(); - hideElem(btnEl); + hideElem(btnEls); } else { const {repo_owner_name, repo_name, issue_index, seconds} = watch; const issueUrl = `${appSubUrl}/${repo_owner_name}/${repo_name}/issues/${issue_index}`; @@ -137,31 +138,28 @@ function updateStopwatchData(data) { const stopwatchIssue = document.querySelector('.stopwatch-issue'); if (stopwatchIssue) stopwatchIssue.textContent = `${repo_owner_name}/${repo_name}#${issue_index}`; updateStopwatchTime(seconds); - showElem(btnEl); + showElem(btnEls); } return Boolean(data.length); } -let updateTimeIntervalId = null; // holds setInterval id when active -function clearStopwatchTimer() { - if (updateTimeIntervalId !== null) { - clearInterval(updateTimeIntervalId); - updateTimeIntervalId = null; +// TODO: This flickers on page load, we could avoid this by making a custom +// element to render time periods. Feeding a datetime in backend does not work +// when time zone between server and client differs. +function updateStopwatchTime(seconds) { + if (!Number.isFinite(seconds)) return; + const datetime = (new Date(Date.now() - seconds * 1000)).toISOString(); + for (const parent of document.querySelectorAll('.header-stopwatch-dot')) { + const existing = parent.querySelector(':scope > relative-time'); + if (existing) { + existing.setAttribute('datetime', datetime); + } else { + const el = document.createElement('relative-time'); + el.setAttribute('format', 'micro'); + el.setAttribute('datetime', datetime); + el.setAttribute('lang', 'en-US'); + el.setAttribute('title', ''); // make show no title and therefor no tooltip + parent.append(el); + } } } -function updateStopwatchTime(seconds) { - const secs = parseInt(seconds); - if (!Number.isFinite(secs)) return; - - clearStopwatchTimer(); - const stopwatch = document.querySelector('.stopwatch-time'); - // TODO: replace with similar to how system status up time is shown - const start = Date.now(); - const updateUi = () => { - const delta = Date.now() - start; - const dur = prettyMilliseconds(secs * 1000 + delta, {compact: true}); - if (stopwatch) stopwatch.textContent = dur; - }; - updateUi(); - updateTimeIntervalId = setInterval(updateUi, 1000); -} diff --git a/web_src/js/modules/tippy.js b/web_src/js/modules/tippy.js index 83b28e5745..a18c94cafb 100644 --- a/web_src/js/modules/tippy.js +++ b/web_src/js/modules/tippy.js @@ -37,8 +37,10 @@ export function createTippy(target, opts = {}) { return onShow?.(instance); }, arrow: arrow || (theme === 'bare' ? false : arrowSvg), - role: role || 'menu', // HTML role attribute - theme: theme || role || 'menu', // CSS theme, either "tooltip", "menu", "box-with-header" or "bare" + // HTML role attribute, ideally the default role would be "popover" but it does not exist + role: role || 'menu', + // CSS theme, either "default", "tooltip", "menu", "box-with-header" or "bare" + theme: theme || role || 'default', plugins: [followCursor], ...other, }); diff --git a/web_src/js/webcomponents/overflow-menu.js b/web_src/js/webcomponents/overflow-menu.js index 0778c5990f..80dd1a545b 100644 --- a/web_src/js/webcomponents/overflow-menu.js +++ b/web_src/js/webcomponents/overflow-menu.js @@ -131,6 +131,7 @@ window.customElements.define('overflow-menu', class extends HTMLElement { interactive: true, placement: 'bottom-end', role: 'menu', + theme: 'menu', content: this.tippyContent, onShow: () => { // FIXME: onShown doesn't work (never be called) setTimeout(() => {