refactor(model,ui): Decouple Yafc.UI.Icon From Yafc.Model via Integer Handle by hcoona · Pull Request #587 · Yafc-CE/yafc-ce

hcoona · 2026-03-02T04:14:17Z

Remove the dependency on Yafc.UI from the domain layer by replacing the Icon enum property in FactorioObject with an integer iconId. This acts as a transitional handle mapped during data loading.

Introduce SystemIcons in Yafc.UI.Core to manage game-sourced system icons (e.g., fuel, warning), mitigating direct usage on DataUtils.
Add FactorioObjectIconExtensions.cs to the application layer, providing GetIcon() to safely resolve iconId values back to Icon representations.
Update UI components to use the new extension method and system icons.
Add architectural XML comments clarifying the temporary placement of these bridging components.

Close #586

hcoona · 2026-03-02T04:14:39Z

Tested for UI surely loaded the icons.

veger · 2026-03-02T08:18:01Z

+            SystemIcons.NoFuelIcon = CreateSimpleIcon(cache, "fuel-icon-red");
+            SystemIcons.WarningIcon = CreateSimpleIcon(cache, "warning-icon");
+            SystemIcons.HandIcon = CreateSimpleIcon(cache, "hand");


I would move this to the SystemIcons class/file, so we just call a setup function and all the (icon) logic is hidden in that function.

Edit: This would also remove the need for [assembly: InternalsVisibleTo("Yafc.Parser")]?

Good point. I've moved the property assignments to a new Initialize method within SystemIcons. This hides the setter logic and, as you noted, removes the need for InternalsVisibleTo.

I kept the icon creation logic in the Parser to maintain the clear separation between data ingestion and UI state management.

Isolate SystemIcons in Yafc.UI.Core to manage game-sourced system icons (e.g., fuel, warning), mitigating direct usage on DataUtils.

… Handle Remove the dependency on Yafc.UI from the domain layer by replacing the Icon enum property in FactorioObject with an integer iconId. This acts as a transitional handle mapped during data loading. - Add FactorioObjectIconExtensions.cs to the application layer, providing GetIcon() to safely resolve iconId values back to Icon representations. - Update UI components to use the new extension method. - Add architectural XML comments clarifying the temporary placement of these bridging components.

DaleStan

Two possible bits of cleanup, to remove some casting (FactorioDataDeserializer.cs) and null-forgiving operators (others).

Other than that, it looks good to me.

DaleStan · 2026-03-17T08:03:44Z

-        }
-    }
+    /// <summary>True when this entry represents a folder/subgroup rather than a project page.</summary>
+    public bool IsSubgroup => subgroup != null;


Suggested change

public bool IsSubgroup => subgroup != null;

[MemberNotNullWhen(true, nameof(subgroup))]

public bool IsSubgroup => subgroup != null;

(Note that this also needs an additional using directive.)

DaleStan · 2026-03-17T08:05:21Z

-                if (gui.BuildButton(row.subgroup.expanded ? Icon.ChevronDown : Icon.ChevronRight)) {
-                    row.subgroup.RecordChange().expanded = !row.subgroup.expanded;
+            if (row.IsSubgroup) {
+                var subgroup = row.subgroup!; // IsSubgroup guarantees subgroup != null


Suggested change

var subgroup = row.subgroup!; // IsSubgroup guarantees subgroup != null

var subgroup = row.subgroup;

DaleStan · 2026-03-17T08:05:41Z

-                if (entry.subgroup.expanded) {
-                    BuildButtons(gui, 1.5f, entry.subgroup);
+            if (entry.IsSubgroup) {
+                var subgroup = entry.subgroup!; // IsSubgroup guarantees subgroup != null


Suggested change

var subgroup = entry.subgroup!; // IsSubgroup guarantees subgroup != null

var subgroup = entry.subgroup;

DaleStan · 2026-03-17T08:07:52Z

+                warningIcon: CreateSimpleIcon(cache, "warning-icon"),
+                handIcon: CreateSimpleIcon(cache, "hand"));

            Dictionary<string, Icon> simpleSpritesCache = [];


Suggested change

Dictionary<string, Icon> simpleSpritesCache = [];

Dictionary<string, int> simpleSpritesCache = [];

DaleStan · 2026-03-17T08:14:42Z

@@ -246,23 +247,23 @@ private void RenderIcons() {
                    bool simpleSprite = o.iconSpec.Length == 1 && o.iconSpec[0].IsSimple();

                    if (simpleSprite && simpleSpritesCache.TryGetValue(o.iconSpec[0].path, out var icon)) {


Suggested change

if (simpleSprite && simpleSpritesCache.TryGetValue(o.iconSpec[0].path, out var icon)) {

if (simpleSprite && simpleSpritesCache.TryGetValue(o.iconSpec[0].path, out int iconId)) {

DaleStan · 2026-03-17T08:14:54Z


                    if (simpleSprite && simpleSpritesCache.TryGetValue(o.iconSpec[0].path, out var icon)) {
-                        o.icon = icon;
+                        o.iconId = (int)icon;


Suggested change

o.iconId = (int)icon;

o.iconId = iconId;

DaleStan · 2026-03-17T08:15:27Z


                        if (simpleSprite) {
-                            simpleSpritesCache[o.iconSpec[0].path] = o.icon;
+                            simpleSpritesCache[o.iconSpec[0].path] = (Icon)o.iconId;


Suggested change

simpleSpritesCache[o.iconSpec[0].path] = (Icon)o.iconId;

simpleSpritesCache[o.iconSpec[0].path] = o.iconId;

DaleStan · 2026-03-17T10:01:37Z

Also, I wrote a Little Program (using LibGit2Sharp) to check the git database for Windows-style line endings in HEAD^{tree}, and it says Yafc.UI/Core/SystemIcons.cs and Yafc/Widgets/FactorioObjectIconExtensions.cs need to be recommitted Unix-style.

using LibGit2Sharp;

SearchFrom(new Repository(/*path to directory that contains .git goes here*/).Head.Tip.Tree);

void SearchFrom(Tree tree) {
    foreach (var item in tree)
        if (item.Target is Blob { IsBinary: false } blob)
            CheckBlob(blob, item.Path);
        else if (item.Target is Tree t)
            SearchFrom(t);
}

void CheckBlob(Blob target, string path) {
    using var stream = target.GetContentStream();
    int b, count = 0;
    while ((b = stream.ReadByte()) != -1)
        if (b == '\r') count++;

    if (count > 0)
        Console.WriteLine($"{path}: {count}");
}

(Does it work with a linked worktree? No idea.)

hcoona requested a review from shpaass as a code owner March 2, 2026 04:14

hcoona force-pushed the dev/shuaizhang/decouple-model-icon branch from cefe754 to 126cc3d Compare March 2, 2026 04:32

veger reviewed Mar 2, 2026

View reviewed changes

hcoona added 2 commits March 2, 2026 22:57

refactor(model,ui): Extract System Icons from Domain Layer Into UI Layer

96c241e

Isolate SystemIcons in Yafc.UI.Core to manage game-sourced system icons (e.g., fuel, warning), mitigating direct usage on DataUtils.

hcoona force-pushed the dev/shuaizhang/decouple-model-icon branch from 126cc3d to 9bc22df Compare March 3, 2026 06:59

hcoona requested a review from veger March 3, 2026 07:02

veger approved these changes Mar 3, 2026

View reviewed changes

DaleStan reviewed Mar 17, 2026

View reviewed changes

	public bool IsSubgroup => subgroup != null;
	[MemberNotNullWhen(true, nameof(subgroup))]
	public bool IsSubgroup => subgroup != null;

	var subgroup = row.subgroup!; // IsSubgroup guarantees subgroup != null
	var subgroup = row.subgroup;

	var subgroup = entry.subgroup!; // IsSubgroup guarantees subgroup != null
	var subgroup = entry.subgroup;

	Dictionary<string, Icon> simpleSpritesCache = [];
	Dictionary<string, int> simpleSpritesCache = [];

		@@ -246,23 +247,23 @@ private void RenderIcons() {
		bool simpleSprite = o.iconSpec.Length == 1 && o.iconSpec[0].IsSimple();

		if (simpleSprite && simpleSpritesCache.TryGetValue(o.iconSpec[0].path, out var icon)) {

	simpleSpritesCache[o.iconSpec[0].path] = (Icon)o.iconId;
	simpleSpritesCache[o.iconSpec[0].path] = o.iconId;

Conversation

hcoona commented Mar 2, 2026

Uh oh!

hcoona commented Mar 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaleStan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaleStan commented Mar 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants