Skip to content

Code review UI improvements and bugfixes #4682

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

Merged
merged 8 commits into from
Sep 17, 2018

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented Aug 12, 2018

  1. Allow review comment/reject without message if code comments have been already added
  2. Dark theme fixes for code comments
  3. Make visually better padding for code comment replies
  4. Do not show button Add single comment if there is already started review
  5. Fixes Cannot add review comment #4901
  6. Fixes Add single comment when reviewing code in deleted file generates fatal: no such path #4752
  7. Fixes Arc Green (dark) theme Review result is near unreadable #4909

@lafriks lafriks added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI labels Aug 12, 2018
@lafriks lafriks added this to the 1.6.0 milestone Aug 12, 2018
@codecov-io
Copy link

Codecov Report

Merging #4682 into master will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4682      +/-   ##
==========================================
+ Coverage   20.62%   20.62%   +<.01%     
==========================================
  Files         166      166              
  Lines       32262    32270       +8     
==========================================
+ Hits         6654     6657       +3     
- Misses      24629    24635       +6     
+ Partials      979      978       -1
Impacted Files Coverage Δ
routers/repo/pull_review.go 0% <0%> (ø) ⬆️
modules/process/manager.go 73.91% <0%> (+4.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4fa6bb...e4a7be9. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 12, 2018
@lunny
Copy link
Member

lunny commented Aug 13, 2018

It seems I cannot post comment on code lines any more.

@lafriks
Copy link
Member Author

lafriks commented Aug 13, 2018

@lunny try Ctrl+F5. What error do you get?

@lunny
Copy link
Member

lunny commented Aug 13, 2018

PANIC: runtime error: index out of range
/Users/xxx/goroot/src/runtime/panic.go:502 (0x402d798)
	gopanic: reflectcall(nil, unsafe.Pointer(d.fn), deferArgs(d), uint32(d.siz), uint32(d.siz))
/Users/xxx/goroot/src/runtime/panic.go:28 (0x402c5dd)
	panicindex: panic(indexError)
/Users/xxx/gopath/src/code.gitea.io/gitea/models/git_diff.go:321 (0x488592f)
	CutDiffAroundLine: begin = com.StrTo(groups[3]).MustInt64()
/Users/xxx/gopath/src/code.gitea.io/gitea/models/issue_comment.go:848 (0x48a30f6)
	CreateCodeComment: patch = CutDiffAroundLine(strings.NewReader(patchBuf.String()), int64((&Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines)
/Users/xxx/gopath/src/code.gitea.io/gitea/routers/repo/pull_review.go:67 (0x4a0a59f)
	CreateCodeComment: comment, err := models.CreateCodeComment(
/Users/xxx/goroot/src/runtime/asm_amd64.s:575 (0x405a211)
	call128: CALLFN(·call128, 128)
/Users/xxx/goroot/src/reflect/value.go:447 (0x40bdde8)
	Value.call: call(frametype, fn, args, uint32(frametype.size), uint32(retOffset))
/Users/xxx/goroot/src/reflect/value.go:308 (0x40bd363)
	Value.Call: return v.call("Call", in)
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/github.com/go-macaron/inject/inject.go:177 (0x43b7634)
	(*injector).callInvoke: return reflect.ValueOf(f).Call(in), nil
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/github.com/go-macaron/inject/inject.go:137 (0x43b6fb9)
	(*injector).Invoke: return inj.callInvoke(f, t, t.NumIn())
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/gopkg.in/macaron.v1/context.go:121 (0x43e386d)
	(*Context).run: vals, err := c.Invoke(c.handler())
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/gopkg.in/macaron.v1/context.go:112 (0x43e37ae)
	(*Context).Next: c.run()
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/github.com/go-macaron/session/session.go:186 (0x4434305)
	Sessioner.func1: ctx.Next()
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/gopkg.in/macaron.v1/context.go:79 (0x43e3680)
	ContextInvoker.Invoke: invoke(params[0].(*Context))
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/github.com/go-macaron/inject/inject.go:157 (0x43b7346)
	(*injector).fastInvoke: return f.Invoke(in)
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/github.com/go-macaron/inject/inject.go:135 (0x43b70aa)
	(*injector).Invoke: return inj.fastInvoke(v, t, t.NumIn())
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/gopkg.in/macaron.v1/context.go:121 (0x43e386d)
	(*Context).run: vals, err := c.Invoke(c.handler())
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/gopkg.in/macaron.v1/context.go:112 (0x43e37ae)
	(*Context).Next: c.run()
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/gopkg.in/macaron.v1/recovery.go:161 (0x43f43da)
	Recovery.func1: c.Next()
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/gopkg.in/macaron.v1/logger.go:40 (0x43e69d3)
	LoggerInvoker.Invoke: invoke(params[0].(*Context), params[1].(*log.Logger))
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/github.com/go-macaron/inject/inject.go:157 (0x43b7346)
	(*injector).fastInvoke: return f.Invoke(in)
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/github.com/go-macaron/inject/inject.go:135 (0x43b70aa)
	(*injector).Invoke: return inj.fastInvoke(v, t, t.NumIn())
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/gopkg.in/macaron.v1/context.go:121 (0x43e386d)
	(*Context).run: vals, err := c.Invoke(c.handler())
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/gopkg.in/macaron.v1/context.go:112 (0x43e37ae)
	(*Context).Next: c.run()
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/gopkg.in/macaron.v1/logger.go:52 (0x43f36f0)
	Logger.func1: ctx.Next()
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/gopkg.in/macaron.v1/logger.go:40 (0x43e69d3)
	LoggerInvoker.Invoke: invoke(params[0].(*Context), params[1].(*log.Logger))
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/github.com/go-macaron/inject/inject.go:157 (0x43b7346)
	(*injector).fastInvoke: return f.Invoke(in)
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/github.com/go-macaron/inject/inject.go:135 (0x43b70aa)
	(*injector).Invoke: return inj.fastInvoke(v, t, t.NumIn())
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/gopkg.in/macaron.v1/context.go:121 (0x43e386d)
	(*Context).run: vals, err := c.Invoke(c.handler())
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/gopkg.in/macaron.v1/router.go:187 (0x43f5581)
	(*Router).Handle.func1: c.run()
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/gopkg.in/macaron.v1/router.go:303 (0x43eef7b)
	(*Router).ServeHTTP: h(rw, req, p)
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/gopkg.in/macaron.v1/macaron.go:220 (0x43e7cc4)
	(*Macaron).ServeHTTP: m.Router.ServeHTTP(rw, req)
/Users/xxx/gopath/src/code.gitea.io/gitea/vendor/github.com/gorilla/context/context.go:141 (0x4752fca)
	ClearHandler.func1: h.ServeHTTP(w, r)
/Users/xxx/goroot/src/net/http/server.go:1947 (0x429f223)
	HandlerFunc.ServeHTTP: f(w, r)
/Users/xxx/goroot/src/net/http/server.go:2694 (0x42a1f7b)
	serverHandler.ServeHTTP: handler.ServeHTTP(rw, req)
/Users/xxx/goroot/src/net/http/server.go:1830 (0x429e240)
	(*conn).serve: serverHandler{c.server}.ServeHTTP(w, w.req)
/Users/xxx/goroot/src/runtime/asm_amd64.s:2361 (0x405c8c0)
	goexit: BYTE	$0x90	// NOP
2018/08/13 17:34:06 http: multiple response.WriteHeader calls
[Macaron] 2018-08-13 17:34:06: Completed POST /lunny/git/pulls/2/files/reviews/comments 500 Internal Server Error in 203.638717ms

@lunny
Copy link
Member

lunny commented Sep 7, 2018

@lafriks conflicted

@lafriks lafriks force-pushed the feat/improve_code_review_ui branch from 0a6ce06 to b6b6018 Compare September 16, 2018 16:08
@lafriks lafriks force-pushed the feat/improve_code_review_ui branch from b6b6018 to 382a558 Compare September 16, 2018 17:14
@lafriks
Copy link
Member Author

lafriks commented Sep 16, 2018

@lunny conflicts resolved. Can you give more details on your error? I'm not able to reproduce it and I don't think it is related to my changes.
That error was not related to my changes but I did fix it I think :)

@lafriks lafriks mentioned this pull request Sep 16, 2018
1 task
@lafriks lafriks changed the title Code review UI improvements Code review UI improvements and bugfixes Sep 17, 2018
@lafriks lafriks requested a review from Morlinest September 17, 2018 04:45
@bkcsoft bkcsoft 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 Sep 17, 2018
@bkcsoft bkcsoft 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 Sep 17, 2018
@lafriks lafriks merged commit 4befec2 into go-gitea:master Sep 17, 2018
@lafriks lafriks deleted the feat/improve_code_review_ui branch September 17, 2018 14:59
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI type/bug type/enhancement An improvement of existing functionality
Projects
None yet
5 participants