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

area/ui: Add a preference to render function names from the right #5382

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions ui/packages/shared/components/src/UserPreferences/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ const UserPreferences = ({modal}: {modal?: boolean}): JSX.Element => {
id="h-highlight-similar-stacks"
userPreferenceDetails={USER_PREFERENCES.HIGHLIGHT_SIMILAR_STACKS}
/>
<UserPreferenceItem
id="h-show-function-name-from-left"
userPreferenceDetails={USER_PREFERENCES.SHOW_FUNCTION_NAME_FROM_LEFT}
/>
<FlamegraphColorProfileSelector />
</div>
</div>
Expand Down
8 changes: 8 additions & 0 deletions ui/packages/shared/hooks/src/useUserPreference/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,20 @@
description:
"When enabled, this option automatically highlights stacks that are similar to the one you're currently hovering over in the Icicle graph.",
},
SHOW_FUNCTION_NAME_FROM_LEFT: {
name: 'Show function name from left side',
key: 'SHOW_FUNCTION_NAME_FROM_LEFT',
type: 'boolean',
default: true,
description:
'When enabled, function names in the graph will be shown starting from the left side. When disabled, names will be shown from the right side, which can be more useful for languages where the most distinctive part of the function name appears at the end.',
},
} as const;

export type UserPreference = keyof typeof USER_PREFERENCES;

const useUserPreference = <T>(preferenceName: UserPreference): [T, (flag: T) => void] => {
const [flags, setFlags] = useLocalStorage<{[flag: string]: any}>(USER_PREFERENCES_KEY, {});

Check warning on line 80 in ui/packages/shared/hooks/src/useUserPreference/index.ts

View workflow job for this annotation

GitHub Actions / UI Test and Lint

Unexpected any. Specify a different type

const value: T = flags[preferenceName] ?? USER_PREFERENCES[preferenceName].default;
const setFlag = (flag: T): void => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import 'react-contexify/dist/ReactContexify.css';

import {ProfileType} from '@parca/parser';

import TextWithEllipsis from './TextWithEllipsis';
import {
FIELD_CHILDREN,
FIELD_CUMULATIVE,
Expand Down Expand Up @@ -412,9 +413,12 @@ export const IcicleNode = React.memo(function IcicleNodeNoMemo({
/>
{width > 5 && (
<svg width={width - 5} height={height}>
<text x={5} y={15} style={{fontSize: '12px'}}>
{name}
</text>
<TextWithEllipsis
text={name}
x={5}
y={15}
width={width - 10} // Subtract padding from available width
/>
</svg>
)}
</g>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
// Copyright 2022 The Parca Authors
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import {useEffect, useRef, useState} from 'react';

import {USER_PREFERENCES, useUserPreference} from '@parca/hooks';

interface Props {
text: string;
x: number;
y: number;
width: number;
}

function calculateTruncatedText(
text: string,
textElement: SVGTextElement,
maxWidth: number
): string {
// Create a temporary element for measurement
const tempElement = textElement.cloneNode() as SVGTextElement;
tempElement.textContent = text;
textElement.parentElement?.appendChild(tempElement);

// If the text fits, return it
const fullWidth = tempElement.getComputedTextLength();
if (fullWidth <= maxWidth) {
textElement.parentElement?.removeChild(tempElement);
return text;
}

// Binary search to find the maximum text that fits
let start = 0;
let end = text.length;
let result = text;

while (start < end) {
const mid = Math.floor((start + end + 1) / 2);
const truncated = text.slice(-mid);

tempElement.textContent = truncated;
const currentWidth = tempElement.getComputedTextLength();

if (currentWidth <= maxWidth) {
result = truncated;
start = mid;
} else {
end = mid - 1;
}
}

textElement.parentElement?.removeChild(tempElement);
return result;
}

function TextWithEllipsis({text, x, y, width}: Props): JSX.Element {
const textRef = useRef<SVGTextElement>(null);
const [displayText, setDisplayText] = useState(text);
const [showFunctionNameFromLeft] = useUserPreference<boolean>(
USER_PREFERENCES.SHOW_FUNCTION_NAME_FROM_LEFT.key
);

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This useState + useEffect combo can be replaced with a single useMemo that returns the text that can be used below to render.

Technically, useState + useEffect method has to render the component twice before it renders the expected output while the useMemo approach would do it in a single render. Not a biggie, but it improves the readability as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, i'll make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm so I made changes based on the feedback above to now use useMemo.

  const displayText = useMemo(() => {
    const textElement = textRef.current;
    if (textElement === null) return text;

    return showFunctionNameFromLeft ? text : calculateTruncatedText(text, textElement, width);
  }, [text, width, showFunctionNameFromLeft, textRef]);

but with this new change we don't get to see the truncated text when it should be shown.

I'm assuming this is related to the usage of the textRef ref and the fact that useMemo is running before the ref is attached to the DOM, therefore the initial render will always have textRef.current as null, and the memoization is preventing subsequent updates.

const textElement = textRef.current;
if (textElement === null) return;

const newText = showFunctionNameFromLeft
? text
: calculateTruncatedText(text, textElement, width);

setDisplayText(newText);
}, [text, width, showFunctionNameFromLeft]);

return (
<text ref={textRef} x={x} y={y} className="text-xs">
{displayText}
</text>
);
}

export default TextWithEllipsis;
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ export const VisualisationToolbar: FC<VisualisationToolbarProps> = ({
{profileViewExternalSubActions != null ? profileViewExternalSubActions : null}
</div>
<div className="flex gap-3">
{preferencesModal != null ? <UserPreferencesModal /> : null}
{preferencesModal === true && <UserPreferencesModal />}
<ShareButton
profileSource={profileSource}
queryClient={queryClient}
Expand Down
Loading