Added progressbar for issues (#1146). #3171

modmew8 · 2017-12-12T14:01:16Z

Hi there,

my pull request adds the progressbar for issues as requested in issue #1146. It basically adds three variables to issue.go that hold the information, how many tasks exist, how many are done, and the percentage of the progress.

The values are updated upon creating a new issue and modifying the issues content. The content is parsed using regular expressions, collecting the amount of valid tasks and valid completed tasks.

The style is github-alike.

I hope I made everything correct - this is the first pull request in my life :) (please be merciful).

Best regards
modmew8

Signed-off-by: modmew8 <[email protected]>

lunny · 2017-12-12T14:42:16Z

Why stored the percent? I think it's unnecessary.

modmew8 · 2017-12-12T15:36:39Z

I could not locate any arithmetic operations in the templates, so I assumed it is task of the models to handle these operations.

As alternative to arithmetic in the templates using go, we could also use css calc():
...style="calc( 100% * {{.Tasksdone}} / {{.Tasks}} );"...

The calculation would be on the clientside this way.

Updating master

Updating progressbarForIssues with latest master

…dded the issue task progress to the user/dashboard/issues. Signed-off-by: modmew8 <[email protected]>

Signed-off-by: modmew8 <[email protected]>

modmew8 · 2017-12-13T15:50:19Z

Removed the percent variable and changed the progress bar to use calc().

Signed-off-by: modmew8 <[email protected]>

ethantkoenig · 2017-12-14T05:24:42Z

models/issue.go

-	Labels          []*Label    `xorm:"-"`
-	MilestoneID     int64       `xorm:"INDEX"`
-	Milestone       *Milestone  `xorm:"-"`
+	Tasks           int


If we're adding new columns to the issue table, then we need a migration.

However, I think the case could be made that we shouldn't store tasks/tasksDone in the database, but instead just calculate them from the Content field.

What do you think about leaving the parsing to the clients? Then we would not have to store anything on the server, and the only files being changed would be the template files (and the css rules). Since it is just a cosmetic addition, the clients could determine the progress themselves upon viewing the issue list - but the server has to provide the content for the clients to parse (additional network load).

I would realize this as a javascript function in the issue list and in the user dashboard issue list.

@ethantkoenig I don't think so. If the page has 10 issues. We have to parse all the 10 issues' markdowns on the browser one time. And also we need add a javascript markdown library. I don't think this is a good idea.

@lunny We don't need to render the issue content as markdown, we just need to look at the raw string (which we load from the DB anyway). The regexps in this PR would not work if the issue contents were rendered.

@ethantkoenig and another thing, we didn't transfer issue content to client on issue list. If you want client to parse that. We have to transfer the contents. This will also reduce the load speed of the page.

I am not suggesting we send the issues' content to the client. That was @modmew8's comment.

I am proposing that we calculate the number of tasks server-side; this can be computed easily with regexps, and does not require rendering the contents as markdown. If our regexps are pre-compiled, I don't think this will be very expensive, since (as I mentioned above) we already load the contents from the DB.

I am hesitant to add new columns to the issue table because I'm afraid that if we will add new columns for every new feature, our schemas will eventually become very complicated and confusing.

ethantkoenig · 2017-12-14T05:24:54Z

models/issue.go

-	MilestoneID     int64       `xorm:"INDEX"`
-	Milestone       *Milestone  `xorm:"-"`
+	Tasks           int
+	Tasksdone       int


TasksDone

ethantkoenig · 2017-12-14T05:25:39Z

models/issue.go

@@ -740,7 +743,16 @@ func AddDeletePRBranchComment(doer *User, repo *Repository, issueID int64, branc
 func (issue *Issue) ChangeContent(doer *User, content string) (err error) {
 	oldContent := issue.Content
 	issue.Content = content
-	if err = UpdateIssueCols(issue, "content"); err != nil {
+
+	regExpTasks := regexp.MustCompile(`(^\s*-\s\[[\sx]\]\s)|(\n\s*-\s\[[\sx]\]\s)`)


This is duplicated code, please consider refactoring.

Also, it would be nice if we didn't have to compile the regexp every time.

…tea-master Merged less changes and updated index.css using make generate-stylesheets.

Merge current Master into fork

Merge pull request #4 from go-gitea/master

codecov-io · 2018-01-02T13:02:38Z

Codecov Report

Merging #3171 into master will decrease coverage by <.01%.
The diff coverage is 62.5%.

@@            Coverage Diff             @@
##           master    #3171      +/-   ##
==========================================
- Coverage   34.82%   34.82%   -0.01%     
==========================================
  Files         277      277              
  Lines       40263    40271       +8     
==========================================
+ Hits        14023    14025       +2     
- Misses      24181    24186       +5     
- Partials     2059     2060       +1

Impacted Files	Coverage Δ
models/issue.go	`44.43% <62.5%> (+0.14%)`	⬆️
modules/process/manager.go	`76.81% <0%> (-4.35%)`	⬇️
models/repo.go	`43.38% <0%> (ø)`	⬆️

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 22a7a7e...ae0d1df. Read the comment docs.

…d regexp. Signed-off-by: modmew8 <[email protected]>

modmew8 · 2018-01-02T14:20:49Z

I implemented the changes @ethantkoenig suggested, now without new variables in issue, with precompiled regexp and computation on demand when accessing either the issue list or the dashboard issue list.

appleboy · 2018-01-02T14:43:31Z

I restarted the build in Drone.

lunny · 2018-01-03T02:02:15Z

LGTM

lunny · 2018-01-03T02:02:27Z

@ethantkoenig needs your approval.

ethantkoenig · 2018-01-03T03:35:53Z

@lunny LGTM

modmew8 added 2 commits December 12, 2017 14:30

Added progressbar for issues (#1146).

904ee41

Updated the generated index.css.

d9f4e82

Signed-off-by: modmew8 <[email protected]>

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 12, 2017

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Dec 12, 2017

lunny added this to the 1.x.x milestone Dec 12, 2017

modmew8 added 4 commits December 13, 2017 16:13

Merge pull request #1 from go-gitea/master

4ca3005

Updating master

Merge pull request #2 from modmew8/master

5d78b7f

Updating progressbarForIssues with latest master

Removed stored progress percentage and changed it to css calc. Also a…

c220a27

…dded the issue task progress to the user/dashboard/issues. Signed-off-by: modmew8 <[email protected]>

Removed unnecessary blanks.

265636c

Signed-off-by: modmew8 <[email protected]>

Formatted the files correctly, fmt-check terminates now without errors.

7ce012b

Signed-off-by: modmew8 <[email protected]>

ethantkoenig suggested changes Dec 14, 2017

View reviewed changes

modmew8 added 4 commits January 2, 2018 13:39

Merge branch 'master' of https://github.com/go-gitea/gitea into go-gi…

74adf36

…tea-master Merged less changes and updated index.css using make generate-stylesheets.

Merge branch 'go-gitea-master' into progressbarForIssues

890349e

Merge pull request #4 from go-gitea/master

2fdeeb5

Merge current Master into fork

Merge pull request #5 from modmew8/master

7088869

Merge pull request #4 from go-gitea/master

Removed variables, made computing the tasks on demand with precompile…

3b7a161

…d regexp. Signed-off-by: modmew8 <[email protected]>

appleboy approved these changes Jan 2, 2018

View reviewed changes

tboerger 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 Jan 2, 2018

lunny modified the milestones: 1.x.x, 1.4.0 Jan 3, 2018

tboerger 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 Jan 3, 2018

ethantkoenig approved these changes Jan 3, 2018

View reviewed changes

Merge branch 'master' into progressbarForIssues

ae0d1df

lunny merged commit d996da6 into go-gitea:master Jan 3, 2018

lunny mentioned this pull request Jan 3, 2018

Progressbar for issues #1146

Closed

modmew8 deleted the progressbarForIssues branch January 3, 2018 16:27

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Uh oh!

Added progressbar for issues (#1146). #3171

Added progressbar for issues (#1146). #3171

Uh oh!

Conversation

modmew8 commented Dec 12, 2017

Uh oh!

lunny commented Dec 12, 2017

Uh oh!

modmew8 commented Dec 12, 2017

Uh oh!

modmew8 commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jan 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

modmew8 commented Jan 2, 2018

Uh oh!

appleboy commented Jan 2, 2018

Uh oh!

lunny commented Jan 3, 2018

Uh oh!

lunny commented Jan 3, 2018

Uh oh!

ethantkoenig commented Jan 3, 2018

Uh oh!

Uh oh!

codecov-io commented Jan 2, 2018 •

edited

Loading