UI: change summary under prefix #2744

johnnyaug · 2021-11-23T09:25:43Z

When viewing a change, this feature adds a button to calculate a summary of the change under every key prefix.
When the button is clicked, the user will see a number of added/changed/deleted/conflicting objects under the prefix.

This was added to all screens where changes can be viewed: Uncommitted changes, Compare view, and when diving into a specific commit.

Solves #2675.

talSofer · 2021-11-23T11:09:30Z

webui/src/lib/components/repository/ChangeSummary.jsx

+class SummaryEntry {
+    constructor() {
+        this.count = 0
+        this.size = 0


Suggested change

this.size = 0

this.sizeBytes = 0

talSofer · 2021-11-23T11:11:25Z

webui/src/lib/components/repository/ChangeSummary.jsx

+                <><span className={"color-fg-removed"}>{summaryData.removed.count}</span> objects removed<br/></>}
+            {summaryData.changed.count > 0 &&


Can you please explain your choice not to show the size for the items removed?

Fixed.

(I was under the false impression that the size wasn't returned for "deleted" type diffs)

talSofer · 2021-11-23T11:25:40Z

webui/src/lib/components/repository/changes.jsx

        pathSection = <Link href={onNavigate(entry)}>{pathSection}</Link>
    }
+    const rowActions = []
+    if (onClickExpandDiff) {
+        rowActions.push(new RowAction(<FileDiffIcon/>, diffExpanded ? "Hide changes" : "Show changes", diffExpanded, onClickExpandDiff))


The FileDiffIcon should be visible by default. I think that you have a bug here because the icon becomes visible only while hovering a leaf row.

Couldn't find a reasonable way to do it, I don't want all the side-actions to be visible by default.

webui/src/lib/components/repository/changes.jsx

itaiad200

Requesting changes mostly for checking consistency with the actual file viewers. If it's too much for this PR let's open a separate issue for consistency and cache handling of entries in the ChangeViewer.

itaiad200 · 2021-11-23T12:25:42Z

webui/src/lib/components/repository/ChangeSummary.jsx

+
+/**
+ * Widget to display a summary of a change: the number of added/changed/deleted/conflicting objects.
+ * Shows an error if the change is has more than {@link MAX_NUM_OBJECTS} entries.


Suggested change

* Shows an error if the change is has more than {@link MAX_NUM_OBJECTS} entries.

* Shows an error if the change has more than {@link MAX_NUM_OBJECTS} entries.

itaiad200 · 2021-11-23T12:35:45Z

webui/src/lib/components/repository/ChangeSummary.jsx

+        setResultsState({results: resultsState.results.concat(results), pagination: pagination, tooManyPages: false})
+    }, [resultsState.results, loading])
+
+    if (loading) return <ClockIcon/>


Why take this approach instead of showing results so far and keep update it forever..?

I think it's not a good experience to show an updating number of objects.

itaiad200 · 2021-11-23T12:44:09Z

webui/src/lib/components/repository/changes.jsx

@@ -89,65 +96,79 @@ export const TreeItem = ({ entry, repo, reference, leftDiffRefID, rightDiffRefID
        return <Error error={error}/>

    if (loading && results.length === 0)
-        return <TreeEntryRow key={entry.path+"entry-row"} entry={entry} showActions={true} loading={true} relativeTo={relativeTo} depth={depth} onRevert={onRevert} onNavigate={onNavigate} repo={repo} reference={reference}/>
+        return <TreeEntryRow key={entry.path+"entry-row"} entry={entry} loading={true} relativeTo={relativeTo} depth={depth} onRevert={onRevert} onNavigate={onNavigate}repo={repo} reference={reference}


Suggested change

return <TreeEntryRow key={entry.path+"entry-row"} entry={entry} loading={true} relativeTo={relativeTo} depth={depth} onRevert={onRevert} onNavigate={onNavigate}repo={repo} reference={reference}

return <TreeEntryRow key={entry.path+"entry-row"} entry={entry} loading={true} relativeTo={relativeTo} depth={depth} onRevert={onRevert} onNavigate={onNavigate} repo={repo} reference={reference}

itaiad200 · 2021-11-23T12:56:00Z

webui/src/lib/components/repository/ChangeSummary.jsx

+    useEffect(async () => {
+        // get pages until reaching the max change size
+        if (resultsState.results && resultsState.results.length >= MAX_NUM_OBJECTS) {
+            setResultsState({results: null, pagination: {}, tooManyPages: true})
+            setLoading(false)
+            return
+        }
+        if (!loading) {
+            return
+        }
+        const {results, pagination} = await getMore(resultsState.pagination.next_offset || "", prefix, false, PAGE_SIZE)
+        if (!pagination.has_more) {
+            setLoading(false)
+        }
+        setResultsState({results: resultsState.results.concat(results), pagination: pagination, tooManyPages: false})
+    }, [resultsState.results, loading])


This is inconsistent with file viewer. You may show statistics for 3 files but present 2 in the ui.
This happens because:

Listing happens separately.

Files are cached and statistics are not.

Solving this basically means I have to refresh all expanded paths in the tree, together with all the stats that are already shown (see explanation below).

I'm not sure it's worth the effort and the performance risk. Since it is clearly shown to the user that the summary is calculated, I think inconsistencies can be expected.

Explanation

Since summaries are calculated on demand - the only way for me to be consistent with the expanded tree is to refresh all expanded paths under the given path.

After doing so, shown summaries on upper levels should be updated to be consistent with their children.
In turn, all expanded paths under these summaries should be updated to be consistent with them.

This potentially requires updating the whole tree.

@johnnyaug to clarify - are you referring to a situation where you added let's say 100 objects to partition "a", and at a time 50 objects are loaded to the tree but the stats show that 100 objects were added?

What is described here is a scenario where data is constantly written to the repo, and you are viewing the change at different points in time.

Just to make sure I understand, this limitation is due to the way we built the tree structure. "Refreshing" means we need to go and fetch everything again, while the summary already did it. I don't want to block on it because I don't think it would be that common. Can we open an issue for consolidation of the summary and the tree fetching?

nopcoder · 2021-11-23T22:00:59Z

webui/src/lib/components/repository/ChangeSummary.jsx

+        this.size = 0
+    }
+    add(count, size) {
+        this.count += count


guess it will be always 1

talSofer · 2021-11-28T15:25:40Z

webui/src/lib/components/repository/changes.jsx

@@ -143,7 +143,7 @@ export const TreeEntryRow = ({entry, relativeTo = "", leaf = false, dirExpanded,
        pathSection = <Link href={onNavigate(entry)}>{pathSection}</Link>
    }
    const rowActions = []
-    if (onClickExpandDiff) {
+    if (onClickExpandDiff && entry.type !== 'conflict') {


You don't have to but consider exporting this and other diff types to the new constants.jsx file

talSofer · 2021-11-28T15:45:44Z

webui/src/lib/components/repository/ChangeSummary.jsx

+    useEffect(async () => {
+        // get pages until reaching the max change size
+        if (resultsState.results && resultsState.results.length >= MAX_NUM_OBJECTS) {
+            setResultsState({results: null, pagination: {}, tooManyPages: true})
+            setLoading(false)
+            return
+        }
+        if (!loading) {
+            return
+        }
+        const {results, pagination} = await getMore(resultsState.pagination.next_offset || "", prefix, false, PAGE_SIZE)
+        if (!pagination.has_more) {
+            setLoading(false)
+        }
+        setResultsState({results: resultsState.results.concat(results), pagination: pagination, tooManyPages: false})
+    }, [resultsState.results, loading])


@johnnyaug to clarify - are you referring to a situation where you added let's say 100 objects to partition "a", and at a time 50 objects are loaded to the tree but the stats show that 100 objects were added?

itaiad200

Approving, great work! I mostly played around with the UI, didn't look too deeply into the code.

talSofer

Awsome feature!

johnnyaug added 5 commits November 21, 2021 12:00

move diff action to actions bar

663ab4f

wip

b4250b1

change summary

5e5a5b4

formatting and small refactor

1786a74

revert formatting changes

220d7ed

johnnyaug added the include-changelog PR description should be included in next release changelog label Nov 23, 2021

johnnyaug changed the title ~~UI/diff prefix stats~~ UI: change summary under prefix for Diff/Compare views. Nov 23, 2021

johnnyaug changed the title ~~UI: change summary under prefix for Diff/Compare views.~~ UI: change summary under prefix for Diff/Compare views Nov 23, 2021

johnnyaug requested review from talSofer and itaiad200 November 23, 2021 09:55

johnnyaug marked this pull request as ready for review November 23, 2021 11:32

tidy

aade0ec

talSofer requested changes Nov 23, 2021

View reviewed changes

itaiad200 requested changes Nov 23, 2021

View reviewed changes

nopcoder reviewed Nov 23, 2021

View reviewed changes

johnnyaug added 3 commits November 28, 2021 12:59

cr fixes

a913853

cr fixes

87589a5

Merge branch 'master' into ui/diff_prefix_stats

9a325b1

johnnyaug requested review from talSofer and itaiad200 November 28, 2021 13:42

talSofer reviewed Nov 28, 2021

View reviewed changes

itaiad200 approved these changes Nov 30, 2021

View reviewed changes

talSofer approved these changes Nov 30, 2021

View reviewed changes

johnnyaug changed the title ~~UI: change summary under prefix for Diff/Compare views~~ UI: change summary under prefix Nov 30, 2021

johnnyaug merged commit 88c2ae2 into master Nov 30, 2021

johnnyaug deleted the ui/diff_prefix_stats branch November 30, 2021 11:41

johnnyaug mentioned this pull request Nov 30, 2021

Inconsistencies between objects viewed and change summary #2766

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: change summary under prefix #2744

UI: change summary under prefix #2744

johnnyaug commented Nov 23, 2021 •

edited

Loading

talSofer Nov 23, 2021

johnnyaug Nov 28, 2021

talSofer Nov 23, 2021

johnnyaug Nov 28, 2021

talSofer Nov 23, 2021

johnnyaug Nov 28, 2021

itaiad200 left a comment

itaiad200 Nov 23, 2021

johnnyaug Nov 28, 2021

itaiad200 Nov 23, 2021

johnnyaug Nov 28, 2021

itaiad200 Nov 23, 2021

johnnyaug Nov 28, 2021

itaiad200 Nov 23, 2021

johnnyaug Nov 28, 2021 •

edited

Loading

talSofer Nov 28, 2021

johnnyaug Nov 28, 2021

itaiad200 Nov 30, 2021

nopcoder Nov 23, 2021

talSofer Nov 28, 2021

talSofer Nov 28, 2021

itaiad200 left a comment

talSofer left a comment

		<><span className={"color-fg-removed"}>{summaryData.removed.count}</span> objects removed<br/></>}
		{summaryData.changed.count > 0 &&

	* Shows an error if the change is has more than {@link MAX_NUM_OBJECTS} entries.
	* Shows an error if the change has more than {@link MAX_NUM_OBJECTS} entries.

	return <TreeEntryRow key={entry.path+"entry-row"} entry={entry} loading={true} relativeTo={relativeTo} depth={depth} onRevert={onRevert} onNavigate={onNavigate}repo={repo} reference={reference}
	return <TreeEntryRow key={entry.path+"entry-row"} entry={entry} loading={true} relativeTo={relativeTo} depth={depth} onRevert={onRevert} onNavigate={onNavigate} repo={repo} reference={reference}

UI: change summary under prefix #2744

UI: change summary under prefix #2744

Conversation

johnnyaug commented Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaiad200 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug Nov 28, 2021 • edited Loading

Choose a reason for hiding this comment

Explanation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaiad200 left a comment

Choose a reason for hiding this comment

talSofer left a comment

Choose a reason for hiding this comment

johnnyaug commented Nov 23, 2021 •

edited

Loading

johnnyaug Nov 28, 2021 •

edited

Loading