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

SLI-1826 Hint resolved issues on Current File Tab #1298

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eray-felek-sonarsource
Copy link
Contributor

@eray-felek-sonarsource eray-felek-sonarsource commented Feb 3, 2025

@eray-felek-sonarsource eray-felek-sonarsource force-pushed the feature/ef/SLI-1826-hint-resolved-issues branch from 3d4e6c5 to 16e049b Compare February 3, 2025 10:40
Copy link
Member

@nquinquenel nquinquenel left a comment

Choose a reason for hiding this comment

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

Few comments

@@ -73,6 +83,18 @@ class TreeSummary(private val project: Project, private val treeContentKind: Tre
return "No ${treeContentKind.displayName}s to display"
}

private fun computeEmptyTextForFiltered(newCodePeriod: String): String {
Copy link
Member

Choose a reason for hiding this comment

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

I would only keep computeEmptyText to handle every cases

@@ -29,6 +29,16 @@ class TreeSummary(private val project: Project, private val treeContentKind: Tre
var text: String = emptyText
private set

fun refresh(filesCount: Int, findingsCount: Int, areThereFilteredIssues: Boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of duplicating all the code of this method, you can can fallback the other refresh to this one, with parameter to false

@@ -143,7 +144,10 @@ private int setFileIssues(VirtualFile file, Iterable<LiveIssue> issues) {
}

var filtered = filter(issues);
var nonFiltered = StreamSupport.stream(issues.spliterator(), false).toList();
Copy link
Member

Choose a reason for hiding this comment

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

I think modifying the Iterable to a list is a bit heavy, probably you can call hasNext on the iterator to check whether it's empty or not

Copy link
Member

Choose a reason for hiding this comment

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

Can also be moved inside the if condition

@@ -73,6 +73,7 @@ public class IssueTreeModelBuilder implements FindingTreeModelBuilder {
private boolean includeLocallyResolvedIssues = false;
private Map<VirtualFile, Collection<LiveIssue>> latestIssues;
private TreeSummary treeSummary;
private boolean areThereFilteredIssues;
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably rename it to hasFilteredIssues for example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants