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

Add comments to commit pages #32384

Open
wants to merge 80 commits into
base: main
Choose a base branch
from
Open

Add comments to commit pages #32384

wants to merge 80 commits into from

Conversation

RedCocoon
Copy link

This PR aims to add comments to the commit page (/reponame/username/commit/{id})

Implementation:

  • Added a Conversation model that is a mostly a copy of Issues but only keeping the parts related to conversations itself
  • Decoupled the front-end of the original Conversation from Issue Page for reusability

Screenshots of implementation:
Desktop
image
Mobile
image

Please do give me feedback in any to get this PR approved.

/claim #4898

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 30, 2024
@algora-pbc algora-pbc bot mentioned this pull request Oct 30, 2024
Open
@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 30, 2024
@github-actions github-actions bot added modifies/translation modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/frontend labels Oct 30, 2024
@RedCocoon
Copy link
Author

Lint is failing due to the duplicate lines, though, due to the way I implemented this I think that would be hard to avoid. If anyone have an idea of what to do short of migrating issues itself to use conversations, please let me know

@lunny
Copy link
Member

lunny commented Oct 30, 2024

Some comments are added for the whole commit, some are for special file and lines. Both kinds of comments are needed.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Oct 30, 2024 •
edited
Loading

Lint is failing due to the duplicate lines, though, due to the way I implemented this I think that would be hard to avoid. If anyone have an idea of what to do short of migrating issues itself to use conversations, please let me know

Well, that's the problem .... Gitea is too big a project, it should keep the code maintainable and testable. If there are a lot of duplicate code and no test, it would break soon. The worse result is that nobody knows how to correctly maintain the code in the future, no bug could be fixed, no feature could be added, any change would introduce regressions ........ no end user likes regressions ..... 😅

@RedCocoon
Copy link
Author

I've ensure that everything functions properly now, and have also did some bare-bone unit tests.
The functionality of adding conversations to commit pages should be working now, minus the Notification services

lunny
lunny reviewed Nov 8, 2024
View reviewed changes
modules/indexer/conversations/bleve/bleve.go Outdated
@@ -0,0 +1,222 @@
// Copyright 2024 The Gitea Authors. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

I don't know why we need indexer for commit comments. when and where will these indexers be used?

@RedCocoon
Copy link
Author

RedCocoon commented Nov 8, 2024 via email

@lunny
Copy link
Member

lunny commented Nov 8, 2024

It's mostly copied over from issues that allow for searching the individual comments. Was it not needed?
…
On Sat, 9 Nov 2024, 02:03 Lunny Xiao, @.> wrote: @.* commented on this pull request. ------------------------------ In modules/indexer/conversations/bleve/bleve.go <#32384 (comment)>: > @@ -0,0 +1,222 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. I don't know why we need indexer for commit comments. when and where will these indexers be used? — Reply to this email directly, view it on GitHub <#32384 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AI6CN52QAQ44RXUMNTTYAIDZ7T4FVAVCNFSM6AAAAABQ4KBBB2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMRUGU3TENBVHA . You are receiving this because you authored the thread.Message ID: @.***>

At least, there doesn’t seem to be a specific search requirement.

@RedCocoon
Copy link
Author

RedCocoon commented Nov 9, 2024 via email

@lunny
Copy link
Member

lunny commented Nov 9, 2024

Hmm, should I remove the indexer, for now?
…
On Sat, 9 Nov 2024, 06:30 Lunny Xiao, @.> wrote: It's mostly copied over from issues that allow for searching the individual comments. Was it not needed? … <#m_4111221246103194699_> On Sat, 9 Nov 2024, 02:03 Lunny Xiao, @.> wrote: @. commented on this pull request. ------------------------------ In modules/indexer/conversations/bleve/bleve.go <#32384 (comment) <#32384 (comment)>>: > @@ -0,0 +1,222 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. I don't know why we need indexer for commit comments. when and where will these indexers be used? — Reply to this email directly, view it on GitHub <#32384 (review) <#32384 (review)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AI6CN52QAQ44RXUMNTTYAIDZ7T4FVAVCNFSM6AAAAABQ4KBBB2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMRUGU3TENBVHA . You are receiving this because you authored the thread.Message ID: @ .> At least, there doesn’t seem to be a specific search requirement. — Reply to this email directly, view it on GitHub <#32384 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AI6CN57V4Z6KKZKS7OYPCK3Z7U3RPAVCNFSM6AAAAABQ4KBBB2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRVHA2DSNZUGE . You are receiving this because you authored the thread.Message ID: @.>

I think yes. Those could be considered in other PRs.

@RedCocoon
Copy link
Author

I've removed files related to indexer now

@RedCocoon
Copy link
Author

Can anyone check on this and let me know if there's anything I need to work on

@delvh
Copy link
Member

delvh commented Nov 21, 2024

What is confusing to me is the following:
I don't understand why this requires duplicating the entire issue commenting system.
Especially since it is so large and deeply ingrained into Gitea's behavior, wouldn't it make more sense to adopt a "pointing" mechanism?
That is, the common parts (comments, reactions, edits, replies, …) become their own table (i.e. termed conversation), and then there is a reference on PR (reviews), commits, and so on, pointing to the conversations in question.

While this leads to a bit more table joining, it is probably the only way forward to improve the Gitea commenting system:

That way, it would be rather easy to additionally support (Team) Discussions or threads, features requests with quite some demand (i.e. #14562, #17858, and probably others I didn't find).
With your approach, those would need to be duplicated as well and will probably never be implemented.

@RedCocoon
Copy link
Author

RedCocoon commented Nov 21, 2024 •
edited
Loading

That is... exactly what I'm doing. It is a duplication right now as I do not want to rewrite issues/pr to use this new conversation because it is outside of the scope of this PR.

It is Issue's conversation that should've been separated in the first place, and that is what I did with mine. My current conversation is exactly written in a way to support pointing to other places in the future.

Part of this effort, for instance, is that right now there is a field on conversation for ConversationType that is mostly unused but could be expanded on in the future to accommodate more types of conversations

lunny
lunny reviewed Nov 28, 2024
View reviewed changes
models/conversations/comment.go
const (
CommentTypeComment CommentType = iota // 0 Plain comment, can be associated with a commit (CommitID > 0) and a line (LineNum > 0)

CommentTypeLock // 1 Lock an conversation, giving only collaborators access
Copy link
Member

Choose a reason for hiding this comment

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

Why we have these types?

lunny
lunny reviewed Nov 28, 2024
View reviewed changes
models/conversations/conversation.go
// Conversation represents a conversation.
type Conversation struct {
ID int64 `xorm:"pk autoincr"`
Index int64 `xorm:"UNIQUE(repo_index)"`
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to have a index?

@RedCocoon
Copy link
Author

RedCocoon commented Nov 28, 2024 via email

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

@lunny lunny lunny left review comments

Assignees
No one assigned
Labels
🙋 Bounty claim lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them modifies/frontend modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/translation size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RedCocoon @lunny @wxiaoguang @delvh @GiteaBot