hansva commented on code in PR #9244: URL: https://github.com/apache/hop/pull/9244#discussion_r4491376246
########## plugins/misc/annotation/src/main/java/org/apache/hop/annotation/AnnotationClicks.java: ########## @@ -0,0 +1,254 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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. + */ + +package org.apache.hop.annotation; + +import org.apache.hop.core.IAttributes; +import org.apache.hop.core.gui.AreaOwner; +import org.apache.hop.core.variables.IVariables; +import org.apache.hop.i18n.BaseMessages; +import org.apache.hop.pipeline.PipelineMeta; +import org.apache.hop.pipeline.transform.TransformMeta; +import org.apache.hop.ui.core.dialog.ErrorDialog; +import org.apache.hop.ui.hopgui.file.pipeline.HopGuiPipelineGraph; +import org.apache.hop.ui.hopgui.file.pipeline.extension.HopGuiPipelineGraphExtension; +import org.apache.hop.ui.hopgui.file.shared.ISnapshotUndoSupport; +import org.apache.hop.ui.hopgui.file.workflow.HopGuiWorkflowGraph; +import org.apache.hop.ui.hopgui.file.workflow.extension.HopGuiWorkflowGraphExtension; +import org.apache.hop.workflow.WorkflowMeta; +import org.apache.hop.workflow.action.ActionMeta; +import org.eclipse.swt.events.MouseEvent; +import org.eclipse.swt.widgets.Shell; + +/** + * Canvas clicks on an annotation callout. The callout sits outside the icon, so the click is + * claimed. + */ +public final class AnnotationClicks { + private static final Class<?> PKG = Annotation.class; + + /** + * True after a press on a callout until the matching release. The UI thread is the only writer. + */ + private static boolean calloutPressed; Review Comment: In Hop Web every user session has its own UI thread, but they all share this static field, so "the UI thread is the only writer" doesn't hold there. When one user presses a callout, the flag is set for everyone. The next mouse-up of another user, anywhere on a pipeline or workflow, then goes through `swallowRelease`, gets `setPreventingDefault(true)`, and the graph calls `clearSettings()` and returns. That user's hop, selection rectangle or move is lost, and a move has no undo step and doesn't mark the file as changed. Could you keep the flag per graph? For example on the canvas, with `canvas.setData("annotation-callout-pressed", Boolean.TRUE)` and reading it back on the mouse-up. Or use a `Collections.synchronizedMap(new WeakHashMap<>())` keyed by the graph. Core keeps the same kind of state per graph, for example `outputDataPressed` in `HopGuiPipelineGraph`. ########## plugins/misc/annotation/src/main/java/org/apache/hop/annotation/AnnotationGeometry.java: ########## @@ -0,0 +1,202 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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. + */ + +package org.apache.hop.annotation; + +import java.util.ArrayList; +import java.util.List; +import java.util.function.ToIntFunction; + +/** Places an annotation balloon outside the icon it is anchored to. */ +public final class AnnotationGeometry { + public static final int GAP_PX = 12; + public static final int PADDING_PX = 6; + public static final int MAX_TEXT_WIDTH_PX = 180; + public static final int RADIUS_PX = 8; + + private AnnotationGeometry() {} + + /** A rectangle in the same coordinate space as the icon. */ + public record Bounds(int x, int y, int width, int height) { + public boolean overlaps(Bounds other) { + return x < other.x + other.width + && x + width > other.x + && y < other.y + other.height + && y + height > other.y; + } + + public int centerX() { + return x + width / 2; + } + + public int centerY() { + return y + height / 2; + } + } + + /** A straight connector between two points. */ + public record Segment(int x1, int y1, int x2, int y2) {} + + /** + * Scales a pixel constant by the canvas magnification. A non-positive magnification is treated as + * 1. + * + * @param pixels the unscaled size + * @param magnification the canvas magnification + * @return at least 1 + */ + public static int scale(int pixels, float magnification) { Review Comment: The zoom is applied twice here. `SwtGc.setTransform` and `SvgGc.setTransform` already scale everything the painter draws with a transform, and `gc.getMagnification()` returns that same factor. `textExtent()` measures in the unscaled space. Multiplying `GAP_PX`, `PADDING_PX`, `RADIUS_PX` and `MAX_TEXT_WIDTH_PX` by the magnification (here, lines 84-85, and in `AnnotationPainter` lines 67, 90 and 110) scales them a second time. The result: - At 200% the gap, padding and corner radius are twice too large compared to the icon and the text, and the text wraps at twice its width. - At 50% the callout is cramped and sits close to the icon. - Exported diagrams with `hop export -m` get the same. Clicks still land, because the click area uses the same rectangle as the drawing. Using the pixel constants as they are should fix it, like the core note painter does with `Const.NOTE_MARGIN`. `scale()` and its tests in `AnnotationGeometryTest` can then go. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
