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

Issue #22: Implement sorting using a struct to hold the state of the sorting #43

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Issue #22: Implement sorting using a struct to hold the state of the sorting #43

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cfc9cbb
Initial solution for toggling sorting profiles by percentage
aidanmatchette Feb 26, 2023
ef0e73b
Create a struct to store the state of the sorting settings
aidanmatchette Feb 27, 2023
e0b0aef
Toggle between sortTypes with "s" and toggle sort order with "!"
aidanmatchette Feb 27, 2023
7e78f9d
Implemented changes to not call call bubbletea framework directly
aidanmatchette Mar 13, 2023
473f319
Small formatting changes
aidanmatchette Mar 13, 2023
16c4dbe
Tests: added testing for sort (asc, dsc, name, percentage)
aidanmatchette Apr 19, 2023
fec4a76
Fix: naming errors for sort testing
aidanmatchette Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 70 additions & 5 deletions internal/model/model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,32 @@ const (
helpStateFull
)

type sortType int

const (
sortStateByName sortType = iota
sortStateByPercentage
//sortStateByStatements
)

type sortOrder bool

const (
ASC sortOrder = true
DSC sortOrder = false
Copy link
Owner

Choose a reason for hiding this comment

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

No need to make these exported if everything else is not exported (use lowercase letters)

)

type SortState struct {
Type sortType
Order sortOrder
}

// New create a new model that can be used directly in the tea framework.
func New(opts ...Option) *Model {
m := &Model{
activeView: activeViewList,
helpState: helpStateShort,
sortState: SortState{Type: sortStateByName, Order: ASC},
Copy link
Owner

Choose a reason for hiding this comment

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

Nice! Great way of keeping things backwards compatible 😼

codeRoot: ".",
list: list.New([]list.Item{}, coverProfileDelegate{}, 0, 0),
}
Expand Down Expand Up @@ -78,9 +99,11 @@ type Model struct {
detectedPackageName string
requestedFiles map[string]bool
filteredLinesByFile map[string][]int
profilesLoaded []*cover.Profile
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think about calling this coverProfiles or loadedProfiles instead?


activeView viewName
helpState helpState
sortState SortState
ready bool

err errorview.Model
Expand Down Expand Up @@ -186,10 +209,8 @@ func (m *Model) onProfilesLoaded(profiles []*cover.Profile) (tea.Model, tea.Cmd)
return m.onError(errNoProfiles{})
}

if m.sortByCoverage {
sort.Slice(profiles, func(i, j int) bool {
return percentCovered(profiles[i]) < percentCovered(profiles[j])
})
if m.sortByCoverage || m.sortState.Type == sortStateByPercentage {
m.sortByPercentage(profiles)
}

m.items = make([]list.Item, len(profiles))
Expand Down Expand Up @@ -254,7 +275,18 @@ func (m *Model) onKeyPressed(key string) (tea.Model, tea.Cmd) {
return m, loadFile(adjustedFileName, item.profile)
}

return m, nil
return m, nil

//toggle on and insiate sortByCoverage (default = Asc)
case "s":
m.toggleSort()
m.Update(m.profilesLoaded)
Copy link
Owner

Choose a reason for hiding this comment

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

I think that internal bubbletea methods should not be called by our code; instead, the framework handles these calls externally, and we only need to react to them. Instead, I suggest to implement a new function updateProfilesList or similar, which checks the current sort state (type + direction), figures out the items in the correct order, and sets them into the list (m.list).

The resulting code should look something like this:

	case "s":
		m.toggleSort()
		return m.updateListItems()

	case "!":
		m.toggleSortOrder()
		return m.updateListItems()

and updateListItems will sort the list based on the state, covert the sorted profiles to list items, and call m.list.setItems.

Copy link
Author

Choose a reason for hiding this comment

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

@orlangure After sorting the list items depending on the SortState, does calling the m.list.SetItems automatically update the UI?

Copy link
Author

Choose a reason for hiding this comment

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

I could use some help and direction on sorting by name. Is it best to sort via m.list or m.loadedProfiles. I apologize, I am new to Golang and I am trying to figure out the best way to do this. Any suggestions are greatly appreciated.

func (m *Model) updateListItems()  tea.Cmd {

    switch m.sortState.Type {
    case sortStateByName:
        //sort by name
    case sortStateByPercentage:
        //sort by percentage
        
    }

    return func() tea.Msg {
        return m.loadedProfiles
    }
}

Copy link
Owner

Choose a reason for hiding this comment

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

After sorting the list items depending on the SortState, does calling the m.list.SetItems automatically update the UI?

Yes, if done properly this will cause the UI to redraw. The flow in bubbletea framework works like "action -> model changes -> rendering", and setting list items is the second step - "model changes". Rendering will follow, and will pick up the updated order of items.

Regarding your second question, below is my draft of the changed functions, based on your code:

func (m *Model) updateListItems() (tea.Model, tea.Cmd) {
	m.sort()

	m.items = make([]list.Item, len(m.profilesLoaded))

	for i, p := range m.profilesLoaded {
		// package name should already be set
		p.FileName = strings.TrimPrefix(p.FileName, m.detectedPackageName+"/")
		m.items[i] = &coverProfile{
			profile:    p,
			percentage: percentCovered(p),
		}
	}

	return m, m.list.SetItems(m.items)
}

func (m *Model) sort() {
	switch m.sortState.Type {
	case sortStateByPercentage:
		m.sortByPercentage()
	case sortStateByName:
		m.sortByName()
	}
}

func (m *Model) sortByPercentage() {
	sort.Slice(m.profilesLoaded, func(i, j int) bool {
		if m.sortState.Order == ASC {
			return percentCovered(m.profilesLoaded[i]) < percentCovered(m.profilesLoaded[j])
		} else {
			return percentCovered(m.profilesLoaded[i]) > percentCovered(m.profilesLoaded[j])
		}
	})
}

func (m *Model) sortByName() {
	sort.Slice(m.profilesLoaded, func(i, j int) bool {
		if m.sortState.Order == ASC {
			return m.profilesLoaded[i].FileName < m.profilesLoaded[j].FileName
		} else {
			return m.profilesLoaded[i].FileName > m.profilesLoaded[j].FileName
		}
	})
}

It is not the most efficient and/or elegant way of solving the problem, but it works. It sorts the "loaded profiles" slice, and then loads the already sorted items into the list.

Copy link
Author

@aidanmatchette aidanmatchette Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Great stuff! I was playing around with this and nearly had this same implementation, just couldn't figure out some of the Golang syntax. Thanks for this assistance. How would you feel about pulling out this code block to have its own function. This same process is currently used twice in the code. Ref onProfilesLoaded line 206.

	m.items = make([]list.Item, len(m.profilesLoaded))

	for i, p := range m.profilesLoaded {
		// package name should already be set
		p.FileName = strings.TrimPrefix(p.FileName, m.detectedPackageName+"/")
		m.items[i] = &coverProfile{
			profile:    p,
			percentage: percentCovered(p),
		}
	}

I will push my current changes and this PR should update so you can see where I am at.
I have created a model_test.go file, but I am not too sure where to start with that.
And then add the button shortcuts to the UI so users will be able to see how to toggle the sort features.

Thanks as always!

return m, nil

case "!":
m.toggleSortOrder()
m.Update(m.profilesLoaded)
return m, nil

case "?":
m.toggleHelp()
Expand All @@ -264,6 +296,38 @@ func (m *Model) onKeyPressed(key string) (tea.Model, tea.Cmd) {
return nil, nil
}

func (m *Model) toggleSortOrder() {
switch m.sortState.Order {
case ASC:
m.sortState.Order = DSC
case DSC:
m.sortState.Order = ASC
}
}

func (m *Model) sortByPercentage(profiles []*cover.Profile) {
sort.Slice(profiles, func(i, j int) bool {
if m.sortState.Order == ASC {
return percentCovered(profiles[i]) > percentCovered(profiles[j])
} else {
return percentCovered(profiles[i]) < percentCovered(profiles[j])
}
})
}

func (m *Model) toggleSort() {
switch m.sortState.Type {
case sortStateByName :
m.sortState.Type = sortStateByPercentage
// sort all profiles by name

case sortStateByPercentage:
m.sortState.Type = sortStateByName
// sort all profiles by Ascend percent if isAscend is true
// else sort by Descend
}
}

func (m *Model) toggleHelp() {
// manage help state globally: allow to extend or hide completely
switch m.helpState {
Expand Down Expand Up @@ -328,6 +392,7 @@ func (m *Model) loadProfiles(codeRoot, profileFilename string) tea.Cmd {

finalProfiles = append(finalProfiles, p)
}
m.profilesLoaded = finalProfiles

return finalProfiles
}
Expand Down