Skip to content

Conversation

@valarnin
Copy link
Collaborator

Process file copied from Cn 7.2 version.

ACT plugin lookup logic changed to use StartsWith, which matches OverlayPlugin's logic.

Have not done any testing with either Global or Tc region yet.

@github-actions github-actions bot added plugin /plugin needs-review Awaiting review labels Dec 11, 2025
@valarnin
Copy link
Collaborator Author

This is now ready for review. Thanks to an extremely helpful Discord user for helping to test this and the OverlayPlugin PRs.

Copy link
Collaborator

@Legends0 Legends0 left a comment

Choose a reason for hiding this comment

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

Code looks good to me.

I preferred the older method of fullname comparison since it ensures it is loading files that we expect, but this change does work. I took a look at the history of the line and it seems it was always a full string comparison since created in 2018 and then later ACT plugin added Korean support with a new dll name and so a user added support for it by adding an additional comparison for the new filename. I couldn't see any discussion at the time of using other methods, only just orders in which the comparisons should be made. It also seems that later on another PR was added that mentioned the removal of needing to check for the Korean name.

@github-actions github-actions bot removed the needs-review Awaiting review label Dec 13, 2025
@valarnin
Copy link
Collaborator Author

I preferred the older method of fullname comparison since it ensures it is loading files that we expect, but this change does work. I took a look at the history of the line and it seems it was always a full string comparison since created in 2018 and then later ACT plugin added Korean support with a new dll name and so a user added support for it by adding an additional comparison for the new filename. I couldn't see any discussion at the time of using other methods, only just orders in which the comparisons should be made. It also seems that later on another PR was added that mentioned the removal of needing to check for the Korean name.

My thought process is "We need OverlayPlugin in order to work, regardless of any other factor, so we should use the same logic as there", paired with "OverlayPlugin changed to using StartsWith 4 years ago in commit OverlayPlugin/OverlayPlugin@034b068".

Maybe it makes more sense for OverlayPlugin to do a more sane check (get the assembly of the plugin, check that it has one of the expected FFXIV_ACT_Plugin classes). OverlayPlugin could also make "get plugin reference" a function on FFXIVRepository as well.

I'm willing to make adjustments as needed / what makes the most sense, but I don't want to hold up TC support for these changes, plus with global 7.4 releasing in a few days I'd rather get TC squared away so my focus can be on updating components for any changes there.

@Legends0
Copy link
Collaborator

My thought process is "We need OverlayPlugin in order to work, regardless of any other factor, so we should use the same logic as there", paired with "OverlayPlugin changed to using StartsWith 4 years ago in commit OverlayPlugin/OverlayPlugin@034b068".

Maybe it makes more sense for OverlayPlugin to do a more sane check (get the assembly of the plugin, check that it has one of the expected FFXIV_ACT_Plugin classes). OverlayPlugin could also make "get plugin reference" a function on FFXIVRepository as well.

I'm willing to make adjustments as needed / what makes the most sense, but I don't want to hold up TC support for these changes, plus with global 7.4 releasing in a few days I'd rather get TC squared away so my focus can be on updating components for any changes there.

Thanks for providing that reference. I had difficulty finding it myself by just a basic search. I agree with not holding up TC support for this type of change.

@valarnin valarnin merged commit cbfcefa into OverlayPlugin:main Dec 13, 2025
9 checks passed
github-actions bot pushed a commit to ShadyWhite/cactbot that referenced this pull request Dec 13, 2025
@xiashtra xiashtra mentioned this pull request Dec 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

plugin /plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants