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.

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

Improve the performance of tree listing #570

Closed
wants to merge 8 commits into from

Conversation

typeless
Copy link
Contributor

@typeless typeless commented Jan 3, 2017

Add two query strings start and count for the home page of repository. If the values are omitted or invalid they are set to 0 and the total number of rows respectively.

For #502

@lunny lunny added this to the 1.1.0 milestone Jan 3, 2017
@lunny lunny added the type/enhancement An improvement of existing functionality label Jan 3, 2017
@@ -40,12 +41,33 @@ func renderDirectory(ctx *context.Context, treeLink string) {
return
}

query := ctx.Req.URL.Query()

var start, count int = 0, 100
Copy link
Member

Choose a reason for hiding this comment

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

I prefer the count could be configured and If we ignored some files we have to give a hint on the UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be better. Let me figure out how to do it properly.


var start, count int = 0, 100

if _start, ok := query["start"]; ok && len(_start) == 1 {
Copy link
Member

Choose a reason for hiding this comment

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

ctx. QueryInt ("start") is more convenient.

}
}

if _count, ok := query["count"]; ok && len(_count) == 1 {
Copy link
Member

Choose a reason for hiding this comment

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

ctx.QueryInt ("count")

entries, err := tree.ListEntries()
if err != nil {
ctx.Handle(500, "ListEntries", err)
return
}
entries.Sort()

entries = entries[start : start+count]
Copy link
Member

Choose a reason for hiding this comment

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

lost sort?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to ftp://www.kernel.org/pub/software/scm/git/docs/git-mktree.html, that has been done by git itself when creating the tree object. And my trivial testing seems to confirm it.

@typeless typeless changed the title Improve the performance of tree listing Improve the performance of tree listing Jan 3, 2017
@typeless typeless changed the title Improve the performance of tree listing Improve the performance of tree listing (WIP) Jan 3, 2017
@typeless typeless changed the title Improve the performance of tree listing (WIP) [WIP] Improve the performance of tree listing Jan 3, 2017
@Bwko
Copy link
Member

Bwko commented Jan 5, 2017

When count is not set it shows no commits. When coun/startt > commits Gitea panics

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 5, 2017
@typeless
Copy link
Contributor Author

typeless commented Jan 6, 2017

Please mark this as work-in-progress, I will finish this PR later (adding front-end controls as @lunny suggested).

@Bwko Thanks for pointing it out.

Edit: Or perhaps I can set the default count to the total number of rows, so it can still work as before and have follow-up PRs of UI supports in the future.

Edit2: The latest commit should fix the bug found by @Bwko

@lunny lunny added the pr/wip This PR is not ready for review label Jan 6, 2017
@Bwko
Copy link
Member

Bwko commented Jan 6, 2017

@typeless Good work 👍 Only the sorting is alphabetically and not folder first.

The sorting done by git-mktree is alphabetical but it doesn't take directories into consideration.
@typeless
Copy link
Contributor Author

typeless commented Jan 7, 2017

@Bwko My bad 😆 , didn't notice the nuances.

@@ -12,6 +12,9 @@ import (
"path"
"strings"

Copy link
Member

Choose a reason for hiding this comment

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

Remove redundant empty line.

count := ctx.QueryInt("count")

// Check invalid values
if (start == 0 && count == 0) || start+count < start || start+count > len(entries) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we replace start+count < start with count < 0?

@lunny
Copy link
Member

lunny commented Jan 23, 2017

@typeless Is this PR ready to review?

@typeless
Copy link
Contributor Author

@lunny The UI part is still missing if that is desired to be included in this PR.

@bkcsoft
Copy link
Member

bkcsoft commented Feb 12, 2017

@typeless UI would be required yes, otherwise there may be issue in the future :( Would be nice to have it fetch and render the next set of files via API, like GitHub has load more entries... when there's to many files 🙂

@bkcsoft
Copy link
Member

bkcsoft commented Feb 12, 2017

You could also just include the base/paginate.tmpl, but then you need to use ?page=0 and a static count 🙁

@lunny
Copy link
Member

lunny commented Feb 14, 2017

@typeless Is this completed?

@typeless
Copy link
Contributor Author

@lunny I don't have a good idea about how the UI design should be done, before starting to get familiar with Semantic UI. Please bear with me as my spare timeslices for OSS projects are very limited recently.

If someone is willing to help implement the web UI (I am really not good at it), that would be appreciated.

@lunny
Copy link
Member

lunny commented Feb 14, 2017

OK. Please give the permissions to let maintainers to push to your branch, it's in this PR right sidebar. And I will move this to v1.2.

@lunny lunny modified the milestones: 1.2.0, 1.1.0 Feb 14, 2017
@typeless
Copy link
Contributor Author

@lunny Thanks. I have the "allow edits from maintainers" checked.

@bkcsoft
Copy link
Member

bkcsoft commented Feb 14, 2017

@typeless @lunny I can look at implementing lazy-loading during the weekend 😄

@bkcsoft bkcsoft self-assigned this Feb 14, 2017
@lunny
Copy link
Member

lunny commented Feb 23, 2017

@bkcsoft any update?

@bkcsoft bkcsoft removed their assignment Apr 7, 2017
@bkcsoft
Copy link
Member

bkcsoft commented Apr 7, 2017

@lunny I've been swamped with work unfortunately 🙁

@lunny lunny modified the milestones: 1.3.0, 1.2.0 Apr 30, 2017
@lunny lunny modified the milestones: 1.3.0, 1.x.x Oct 10, 2017
@typeless typeless changed the title [WIP] Improve the performance of tree listing Improve the performance of tree listing Dec 22, 2018
@typeless typeless closed this Dec 22, 2018
@lunny lunny removed this from the 1.x.x milestone Dec 23, 2018
@typeless typeless deleted the limit-commit-log branch April 3, 2019 04: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/need 2 This PR needs two approvals by maintainers to be considered for merging. pr/wip This PR is not ready for review type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants