JNG-5887 refactor icon transformation #313
Conversation
noherczeg
requested review from
robertcsakany,
bencegelei,
gaborflorian and
Madmat8
WalkthroughThe pull request removes a dependency on an external icon file by deleting its import statement and the associated synthetic icon creation function. In its place, two new transformation rules have been introduced to handle icon creation within the UI framework. These rules adjust the process of assigning icons to UI elements by leveraging properties from a UIViewLinkDeclaration, thereby streamlining the icon generation mechanism. Changes
Sequence Diagram(s)sequenceDiagram
participant VLD as UIViewLinkDeclaration
participant TE as Transformation Engine
participant UN as UnsetButtonIcon Rule
participant OS as OpenSetSelectorIcon Rule
participant Icon as UI!ui::Icon
VLD->>TE: Request icon transformation
alt Condition: Unset Button
TE->>UN: Apply Unset Button icon rule
UN->>Icon: Create & configure UI icon
else Condition: OpenSetSelector Button
TE->>OS: Apply OpenSetSelector icon rule
OS->>Icon: Create & configure UI icon
end
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewLinkDeclaration.etl (2)
313-314: Inconsistent naming pattern for UI icon.The name format
s.getFqName() + "::link-off::" + "Icon"differs from other icon naming patterns in this file. For example, line 202 usess.name + "OpenPageIcon". Consider following the established pattern for consistency.- t.name = s.getFqName() + "::link-off::" + "Icon"; + t.name = s.name + "UnsetIcon";
351-352: Inconsistent naming pattern for UI icon.The name format
s.getFqName() + "::link::" + "Icon"differs from other icon naming patterns in this file. For example, line 202 usess.name + "OpenPageIcon". Consider following the established pattern for consistency.- t.name = s.getFqName() + "::link::" + "Icon"; + t.name = s.name + "SetSelectorIcon";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/_importUI.eol(0 hunks)judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/icon.eol(0 hunks)judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewLinkDeclaration.etl(2 hunks)
💤 Files with no reviewable changes (2)
- judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/_importUI.eol
- judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/icon.eol
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ⏳ Build, test and deploy artifacts
🔇 Additional comments (4)
judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewLinkDeclaration.etl (4)
302-302: Icon assignment updated to use the new dedicated icon rule.The code now uses
s.equivalent("ViewLinkDeclarationUnsetButtonIcon")rather than directly creating an icon or using an external method. This is part of the icon transformation refactoring.
340-340: Icon assignment updated to use the new dedicated icon rule.The code now uses
s.equivalent("ViewLinkDeclarationOpenSetSelectorButtonIcon")rather than directly creating an icon or using an external method, which aligns with the refactoring approach.
346-355: New ViewLinkDeclarationOpenSetSelectorButtonIcon rule uses inconsistent variable.Similar to the previous icon rule, this one also uses
actorDeclaration.namefor the icon's ID while other rules userootMenu.name. This inconsistency might cause issues if these variables have different values.See the verification script in the previous comment about
actorDeclarationvsrootMenu.name.
308-317:❓ Verification inconclusive
New ViewLinkDeclarationUnsetButtonIcon rule uses inconsistent variable.
There's an inconsistency in variable usage. The rule uses
actorDeclaration.namefor the icon's ID, while other rules in this file userootMenu.name(see lines like 200, 238, etc.).
🏁 Script executed:
#!/bin/bash # Check if actorDeclaration is defined elsewhere and if rootMenu.name should be used instead # Look for actorDeclaration references echo "Checking actorDeclaration references:" rg -A 2 -B 2 "actorDeclaration" --type epsilon # Look for patterns of icon ID setting in other rules echo "Checking icon ID setting patterns:" rg "setId.*Icon" --type epsilonLength of output: 308
Below is a revised plan to help verify the issue. It looks like the original script’s use of the
--type epsilonoption prevented us from obtaining meaningful output. We need to check the usage of the identifiers in the file without filtering by file type. Please run the following script manually to verify that the file
judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewLinkDeclaration.etluses
actorDeclarationonly in the new rule while earlier rules userootMenu:Once you’ve confirmed whether the only occurrence in the new rule is indeed using
actorDeclaration.name(while the other parts of the file userootMenu.name), please update the rule accordingly. For example, if maintaining consistency is the goal, you should change:- t.setId(actorDeclaration.name + "/(jsl/" + s.getId() + ")/ViewLinkDeclarationUnsetButtonIcon"); + t.setId(rootMenu.name + "/(jsl/" + s.getId() + ")/ViewLinkDeclarationUnsetButtonIcon");
Action Required: Verify Inconsistent Variable Usage in ViewLinkDeclarationUnsetButtonIcon Rule
- Observation: The new rule assigns the icon’s ID using
actorDeclaration.name, whereas similar rules in the file rely onrootMenu.name.- Recommendation: Confirm if this difference is intentional. If consistency is desired, update the rule to use
rootMenu.name.
🏁 Script executed:
#!/bin/bash echo "Searching for 'actorDeclaration' in viewLinkDeclaration.etl:" rg -n "actorDeclaration" "judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewLinkDeclaration.etl" echo "Searching for 'rootMenu' in viewLinkDeclaration.etl:" rg -n "rootMenu" "judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewLinkDeclaration.etl"Length of output: 3433
ACTION: Confirm and Correct Variable Usage for Icon ID in ViewLinkDeclarationUnsetButtonIcon Rule
The new rule sets the icon’s ID using
actorDeclaration.name, whereas all other similar rules in the file userootMenu.name. This inconsistency could affect the behavior of the generated IDs. Please verify whether the use ofactorDeclarationis intentional. If consistency is desired, update the rule as follows:
- File:
judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewLinkDeclaration.etl- Suggested change (around line 312):
- t.setId(actorDeclaration.name + "/(jsl/" + s.getId() + ")/ViewLinkDeclarationUnsetButtonIcon"); + t.setId(rootMenu.name + "/(jsl/" + s.getId() + ")/ViewLinkDeclarationUnsetButtonIcon");Additionally, note that a similar usage of
actorDeclarationappears at line 350 for theViewLinkDeclarationOpenSetSelectorButtonIconrule. Please review if this is also intended to remain or should be updated for consistency.
No description provided.