Skip to content

[JEWEL-1205] Fix Menu Items Looking Too Tall in Presentation Mode #3419

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

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

[JEWEL-1205] Fix Menu Items Looking Too Tall in Presentation Mode #3419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions platform/jewel/ide-laf-bridge/api-dump.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ f:org.jetbrains.jewel.bridge.BridgeUtilsKt
- sf:createVerticalBrush-8A-3gB4(java.util.List,F,F,I):androidx.compose.ui.graphics.Brush
- bs:createVerticalBrush-8A-3gB4$default(java.util.List,F,F,I,I,java.lang.Object):androidx.compose.ui.graphics.Brush
- sf:getDp(com.intellij.util.ui.JBValue):F
- sf:getUnscaledDp(I):F
- sf:retrieveArcAsCornerSize(java.lang.String):androidx.compose.foundation.shape.CornerSize
- sf:retrieveArcAsCornerSizeOrDefault(java.lang.String,androidx.compose.foundation.shape.CornerSize):androidx.compose.foundation.shape.CornerSize
- sf:retrieveArcAsCornerSizeWithFallbacks(java.lang.String[]):androidx.compose.foundation.shape.CornerSize
Expand Down
11 changes: 11 additions & 0 deletions platform/jewel/ide-laf-bridge/src/main/kotlin/org/jetbrains/jewel/bridge/BridgeUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import com.intellij.ui.scale.JBUIScale.scale
import com.intellij.util.ui.JBDimension
import com.intellij.util.ui.JBFont
import com.intellij.util.ui.JBInsets
import com.intellij.util.ui.JBUI
import com.intellij.util.ui.JBValue
import java.awt.Dimension
import java.awt.Insets
Expand Down Expand Up @@ -520,3 +521,13 @@ public fun retrieveEditorColorScheme(): EditorColorsScheme {
val manager = EditorColorsManager.getInstance() as EditorColorsManagerImpl
return manager.schemeManager.activeScheme ?: DefaultColorSchemesManager.getInstance().firstScheme
}

/**
* Converts a raw Swing dimension (which might already be scaled by JBUI) into a Compose Dp.
*
* Use this whenever you fetch an Int value from a scaled property in JBUI. If you don't, you'll get "Double Scaling":
Copy link
Collaborator

@daaria-s daaria-s Feb 12, 2026

Choose a reason for hiding this comment

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

just curious, is it true that there might be more places where we need to use it but we don't know about them?

Copy link
Collaborator Author

@DanielSouzaBertoldi DanielSouzaBertoldi Feb 13, 2026

Choose a reason for hiding this comment

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

Not that I was able to find yet 😮

We do fetch the rowHeight() value in our readFromLaF() function over in BridgeGlobalMetrics but we are already unscaling it there

Copy link
Collaborator

@rock3r rock3r Mar 16, 2026

Choose a reason for hiding this comment

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

Hmm we do unscaling for Insets in this file *see Insets.toPaddingValues()), so if the Ints we get are indeed scaled, we need to check. I don't know if it's a potential issue, but it seems like retrieveIntAsDp*()s do need a closer look to validate whether they're fetching the right values or not.

Otherwise you're saying "it's fine to use retrieveIntAsDp*s everywhere except in this one case where it would do a double scale", which seems a bit odd in principle. Effectively, JBUI.CurrentTheme.List.rowHeight() does the same thing we do, calling JBUI.getInt() which calls UIManager.get() — retrieveIntAsDp*() also does the same, then calling .dp on the value (which is where the double scaling may happen).

* 1. JBUI scales the value (e.g. 24px -> 42px for Presentation Mode)
* 2. Compose scales it AGAIN via LocalDensity (e.g. 42px -> 147px)
*/
public val Int.unscaledDp: Dp
get() = JBUI.unscale(this).dp
3 changes: 2 additions & 1 deletion .../jewel/ide-laf-bridge/src/main/kotlin/org/jetbrains/jewel/bridge/theme/IntUiBridgeMenu.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import org.jetbrains.jewel.bridge.retrieveColorOrUnspecified
import org.jetbrains.jewel.bridge.retrieveIntAsNonNegativeDpOrUnspecified
import org.jetbrains.jewel.bridge.safeValue
import org.jetbrains.jewel.bridge.toPaddingValues
import org.jetbrains.jewel.bridge.unscaledDp
import org.jetbrains.jewel.ui.component.styling.MenuColors
import org.jetbrains.jewel.ui.component.styling.MenuIcons
import org.jetbrains.jewel.ui.component.styling.MenuItemColors
Expand Down Expand Up @@ -107,7 +108,7 @@ internal fun readMenuStyle(): MenuStyle {
iconSize = 16.dp,
minHeight =
if (isNewUiTheme()) {
JBUI.CurrentTheme.List.rowHeight().dp.safeValue()
JBUI.CurrentTheme.List.rowHeight().unscaledDp.safeValue()
} else {
Dp.Unspecified
},
Expand Down
239 changes: 118 additions & 121 deletions platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/Menu.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import androidx.compose.runtime.CompositionLocalProvider
import androidx.compose.runtime.DisposableEffect
import androidx.compose.runtime.Immutable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.MutableState
import androidx.compose.runtime.derivedStateOf
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
Expand Down Expand Up @@ -67,7 +68,6 @@ import org.jetbrains.annotations.ApiStatus
import org.jetbrains.jewel.foundation.GenerateDataFunctions
import org.jetbrains.jewel.foundation.InternalJewelApi
import org.jetbrains.jewel.foundation.modifier.onHover
import org.jetbrains.jewel.foundation.modifier.thenIf
import org.jetbrains.jewel.foundation.state.CommonStateBitMask.Active
import org.jetbrains.jewel.foundation.state.CommonStateBitMask.Enabled
import org.jetbrains.jewel.foundation.state.CommonStateBitMask.Focused
Expand All @@ -87,9 +87,7 @@ import org.jetbrains.jewel.ui.component.styling.LocalMenuStyle
import org.jetbrains.jewel.ui.component.styling.MenuItemColors
import org.jetbrains.jewel.ui.component.styling.MenuItemMetrics
import org.jetbrains.jewel.ui.component.styling.MenuStyle
import org.jetbrains.jewel.ui.disabledAppearance
import org.jetbrains.jewel.ui.icon.IconKey
import org.jetbrains.jewel.ui.painter.hints.Stateful
import org.jetbrains.jewel.ui.popupShadowAndBorder
import org.jetbrains.jewel.ui.theme.menuStyle
import org.jetbrains.skiko.hostOs
Expand Down Expand Up @@ -821,36 +819,18 @@ internal fun MenuItemBase(
style: MenuStyle = JewelTheme.menuStyle,
@Suppress("DEPRECATION") content: @Composable (itemState: MenuItemState) -> Unit,
) {
var itemState by
remember(interactionSource) {
@Suppress("DEPRECATION") mutableStateOf(MenuItemState.of(selected = selected, enabled = enabled))
}

remember(enabled, selected) { itemState = itemState.copy(selected = selected, enabled = enabled) }
val itemState by rememberMenuItemState(selected, enabled, interactionSource)

val focusRequester = remember { FocusRequester() }
val menuController = LocalMenuController.current
val localInputModeManager = LocalInputModeManager.current

LaunchedEffect(interactionSource) {
interactionSource.interactions.collect { interaction ->
when (interaction) {
is PressInteraction.Press -> itemState = itemState.copy(pressed = true)
is PressInteraction.Cancel,
is PressInteraction.Release -> itemState = itemState.copy(pressed = false)
is HoverInteraction.Enter -> {
itemState = itemState.copy(hovered = true)
focusRequester.requestFocus()
}

is HoverInteraction.Exit -> itemState = itemState.copy(hovered = false)
is FocusInteraction.Focus -> itemState = itemState.copy(focused = true)
is FocusInteraction.Unfocus -> itemState = itemState.copy(focused = false)
}
LaunchedEffect(itemState.isHovered) {
if (itemState.isHovered) {
focusRequester.requestFocus()
}
}

val menuController = LocalMenuController.current
val localInputModeManager = LocalInputModeManager.current

Box(
modifier =
modifier
Expand All @@ -872,52 +852,24 @@ internal fun MenuItemBase(
onDispose {}
}

val itemColors = style.colors.itemColors
val itemMetrics = style.metrics.itemMetrics

@Suppress("DEPRECATION") // Not really deprecated, will be made internal
val updatedTextStyle = LocalTextStyle.current.copy(color = itemColors.contentFor(itemState).value)

@Suppress("DEPRECATION") // Not really deprecated, will be made internal
CompositionLocalProvider(
LocalContentColor provides itemColors.contentFor(itemState).value,
LocalTextStyle provides updatedTextStyle,
) {
val backgroundColor by itemColors.backgroundFor(itemState)

Row(
modifier =
Modifier.fillMaxWidth()
.defaultMinSize(minHeight = itemMetrics.minHeight)
.drawItemBackground(itemMetrics, backgroundColor)
.padding(itemMetrics.contentPadding),
horizontalArrangement = Arrangement.spacedBy(4.dp),
verticalAlignment = Alignment.CenterVertically,
) {
if (canShowIcon) {
val iconModifier = Modifier.size(style.metrics.itemMetrics.iconSize)
if (iconKey != null) {
Icon(
key = iconKey,
contentDescription = null,
modifier = iconModifier.thenIf(!enabled) { disabledAppearance() },
Copy link
Collaborator

@rock3r rock3r Mar 16, 2026

Choose a reason for hiding this comment

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

I think we've lost the disabledAppearance here, unless I'm missing something

MenuItemLayout(
itemState = itemState,
style = style,
iconKey = iconKey,
canShowIcon = canShowIcon,
trailingContent =
if (canShowKeybinding) {
{
@Suppress("DEPRECATION") // Not really deprecated, MenuItemColors will be made internal
Text(
modifier = Modifier.padding(style.metrics.itemMetrics.keybindingsPadding),
text = keybindingHint,
color = style.colors.itemColors.keybindingTintFor(itemState).value,
)
} else {
Box(modifier = iconModifier)
}
}

Box(modifier = Modifier.weight(1f, true)) { content(itemState) }

if (canShowKeybinding) {
Text(
modifier = Modifier.padding(style.metrics.itemMetrics.keybindingsPadding),
text = keybindingHint,
color = itemColors.keybindingTintFor(itemState).value,
)
}
}
}
} else null,
content = { content(itemState) },
)
}
}

Expand Down Expand Up @@ -951,43 +903,15 @@ internal fun MenuSubmenuItem(
style: MenuStyle = JewelTheme.menuStyle,
@Suppress("DEPRECATION") content: @Composable (itemState: MenuItemState) -> Unit,
) {
var itemState by
remember(interactionSource) {
@Suppress("DEPRECATION") mutableStateOf(MenuItemState.of(selected = selected, enabled = enabled))
}

remember(enabled) { itemState = itemState.copy(selected = false, enabled = enabled) }

var itemState by rememberMenuItemState(selected, enabled, interactionSource)
val focusRequester = remember { FocusRequester() }

LaunchedEffect(interactionSource) {
interactionSource.interactions.collect { interaction ->
when (interaction) {
is PressInteraction.Press -> itemState = itemState.copy(pressed = true)
is PressInteraction.Cancel,
is PressInteraction.Release -> itemState = itemState.copy(pressed = false)

is HoverInteraction.Enter -> itemState = itemState.copy(hovered = true)
is HoverInteraction.Exit -> itemState = itemState.copy(hovered = false)
is FocusInteraction.Focus -> itemState = itemState.copy(focused = true)
is FocusInteraction.Unfocus -> itemState = itemState.copy(focused = false)
}
}
}

remember(selected) { itemState = itemState.copy(selected = selected) }
LaunchedEffect(itemState.isSelected) { if (itemState.isSelected) focusRequester.requestFocus() }

val itemColors = style.colors.itemColors
val menuMetrics = style.metrics

@Suppress("DEPRECATION") // Not really deprecated, will be made internal
val backgroundColor by itemColors.backgroundFor(itemState)
Box(
modifier =
modifier
.fillMaxWidth()
.drawItemBackground(menuMetrics.itemMetrics, backgroundColor)
.focusRequester(focusRequester)
.clickable(
onClick = { itemState = itemState.copy(selected = !itemState.isSelected) },
Expand All @@ -1004,32 +928,22 @@ internal fun MenuSubmenuItem(
}
}
) {
@Suppress("DEPRECATION") // Not really deprecated, will be made internal
CompositionLocalProvider(LocalContentColor provides itemColors.contentFor(itemState).value) {
Row(
Modifier.fillMaxWidth().padding(menuMetrics.itemMetrics.contentPadding),
verticalAlignment = Alignment.CenterVertically,
horizontalArrangement = Arrangement.spacedBy(4.dp),
) {
if (showIcon) {
if (iconKey != null) {
Icon(key = iconKey, contentDescription = null)
} else {
Box(Modifier.size(style.metrics.itemMetrics.iconSize))
}
}

Box(Modifier.weight(1f)) { content(itemState) }

MenuItemLayout(
itemState = itemState,
style = style,
iconKey = iconKey,
canShowIcon = showIcon,
trailingContent = {
@Suppress("DEPRECATION") // Not really deprecated, MenuItemColors will be made internal
Icon(
key = style.icons.submenuChevron,
tint = itemColors.iconTintFor(itemState).value,
contentDescription = null,
modifier = Modifier.size(style.metrics.itemMetrics.iconSize),
hint = Stateful(itemState),
tint = style.colors.itemColors.iconTintFor(itemState).value,
)
}
}
},
content = { content(itemState) },
)

if (itemState.isSelected) {
Submenu(
Expand All @@ -1048,6 +962,60 @@ internal fun MenuSubmenuItem(
}
}

@Suppress("DEPRECATION") // Not really deprecated, MenuItemColors be made internal
@Composable
internal fun MenuItemLayout(
itemState: MenuItemState,
modifier: Modifier = Modifier,
style: MenuStyle = JewelTheme.menuStyle,
iconKey: IconKey? = null,
canShowIcon: Boolean = true,
trailingContent: (@Composable () -> Unit)? = null,
content: @Composable () -> Unit,
) {
val itemColors = style.colors.itemColors
val itemMetrics = style.metrics.itemMetrics

val contentColor = itemColors.contentFor(itemState).value
val backgroundColor by itemColors.backgroundFor(itemState)

CompositionLocalProvider(
LocalContentColor provides contentColor,
LocalTextStyle provides LocalTextStyle.current.copy(color = contentColor),
) {
Row(
modifier =
modifier
.fillMaxWidth()
.defaultMinSize(minHeight = itemMetrics.minHeight)
.drawItemBackground(itemMetrics, backgroundColor)
.padding(itemMetrics.contentPadding),
horizontalArrangement = Arrangement.spacedBy(4.dp),
verticalAlignment = Alignment.CenterVertically,
) {
if (canShowIcon) {
val iconModifier = Modifier.size(itemMetrics.iconSize)
if (iconKey != null) {
Icon(
key = iconKey,
contentDescription = null,
modifier = iconModifier,
tint = itemColors.iconTintFor(itemState).value,
)
} else {
Box(modifier = iconModifier)
}
}

Box(modifier = Modifier.weight(1f)) { content() }

if (trailingContent != null) {
trailingContent()
}
}
}
}

private fun Modifier.drawItemBackground(itemMetrics: MenuItemMetrics, backgroundColor: Color) = drawBehind {
val cornerSizePx = itemMetrics.selectionCornerSize.toPx(size, density = this)
val cornerRadius = CornerRadius(cornerSizePx, cornerSizePx)
Expand Down Expand Up @@ -1201,3 +1169,32 @@ public value class MenuItemState(public val state: ULong) : SelectableComponentS
}
}
}

@Suppress("DEPRECATION") // Not really deprecated, MenuItemState will be made internal
@Composable
internal fun rememberMenuItemState(
Copy link
Collaborator

@rock3r rock3r Mar 16, 2026

Choose a reason for hiding this comment

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

Wouldn't this subtly change MenuSubmenuItem behavior? Before the refactor it reset selected = false on enabled changes, whereas the shared helper now preserves selected. So if a submenu is currently open / hover-selected and then becomes disabled, I think it would stay selected (and keep the submenu open) until something else dismisses it. Is that intentional?

selected: Boolean,
enabled: Boolean,
interactionSource: MutableInteractionSource,
): MutableState<MenuItemState> {
val itemState =
remember(interactionSource) { mutableStateOf(MenuItemState.of(selected = selected, enabled = enabled)) }

remember(enabled, selected) { itemState.value = itemState.value.copy(selected = selected, enabled = enabled) }

LaunchedEffect(interactionSource) {
interactionSource.interactions.collect { interaction ->
when (interaction) {
is PressInteraction.Press -> itemState.value = itemState.value.copy(pressed = true)
is PressInteraction.Cancel,
is PressInteraction.Release -> itemState.value = itemState.value.copy(pressed = false)
is HoverInteraction.Enter -> itemState.value = itemState.value.copy(hovered = true)
is HoverInteraction.Exit -> itemState.value = itemState.value.copy(hovered = false)
is FocusInteraction.Focus -> itemState.value = itemState.value.copy(focused = true)
is FocusInteraction.Unfocus -> itemState.value = itemState.value.copy(focused = false)
}
}
}

return itemState
}
3 changes: 3 additions & 0 deletions plugins/devkit/intellij.devkit.compose/resources/messages/DevkitComposeBundle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ jewel.swing.editable.disabled=Editable + Disabled
jewel.swing.text.areas=Text areas:
jewel.swing.label=Swing
jewel.compose.label=Compose
jewel.section.menu.label=Menus:
jewel.section.menu.swing.button=Open IntelliJ Platform Menu
jewel.section.menu.swing.submenu=Submenu

compose.sandbox=Compose Sandbox
compose.sandbox.show.automatically.on.project.open=Show automatically on project open
Expand Down
Loading
Loading