Skip to content

Language settings autoupdate#63

Open
tpolyakgs wants to merge 12 commits intomainfrom
language-settings-autoupdate
Open

Language settings autoupdate#63
tpolyakgs wants to merge 12 commits intomainfrom
language-settings-autoupdate

Conversation

@tpolyakgs
Copy link

ACP-22926

As per request from AI solutions automatic update

  • Add an optional autoupdate flag to the Version resource on Windows, and to the Info.plist on macOS to enable easier, resource-based checking of an add-on’s update behavior. This optional flag should come from config.json, its value can be 0 or 1.
  • Support proper language subfolder creation in the pipeline, as currently it stores resources in ‘English’ subfolders, even in localized add-ons
  • Support proper Windows language and charset code in Version resource in localized add-ons.
  • Store GS Loc code in resource STRS 18000

Copy link
Contributor

@vhorvath-gs vhorvath-gs left a comment

Choose a reason for hiding this comment

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

Some minor tweaks.

AddOn.rc.in Outdated
Comment on lines 9 to 11
3 + 1, /* VERSION_APPENDIX length */
0
"@addOnLanguage@" "\0" /* Version appendix for localized data */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
3 + 1, /* VERSION_APPENDIX length */
0
"@addOnLanguage@" "\0" /* Version appendix for localized data */
4,
0
"@addOnLanguage@\0"

Prefer 4 spaces to be in line with VersionInfo.rc.in.

That stray 0 looks odd. Doesn't look like anything in the documentation https://learn.microsoft.com/en-us/windows/win32/menurc/user-defined-resource

Prefer to do the length calculation for the string in CMake.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 160 to +166
set (out "${CMAKE_CURRENT_BINARY_DIR}/${target}-VersionInfo.rc")
configure_file ("${CMAKE_CURRENT_FUNCTION_LIST_DIR}/VersionInfo.rc.in" "${out}" @ONLY)
target_sources ("${target}" PRIVATE "${out}")

set (addOnRes "${CMAKE_CURRENT_BINARY_DIR}/${target}-AddOn.rc")
configure_file ("${CMAKE_CURRENT_FUNCTION_LIST_DIR}/AddOn.rc.in" "${addOnRes}" @ONLY)

target_sources ("${target}" PRIVATE "${out}" "${addOnRes}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Turn this into a loop with foreach (res IN ITEMS VersionInfo AddOn).

endfunction ()

function (generate_add_on_version_info outSemver)
function (generate_add_on_version_info outSemver addOnLanguage)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the function is internal I almost feel like this parameter can be omitted, since it's present in the calling context anyway.

Either that or move it before the out parameter.

endif ()
endif ()
generate_add_on_version_info (semver)
generate_add_on_version_info (semver ${addOnLanguage})
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer "${...}" for things you intend to pass as a single argument and where you don't want list expansion.

# optional members (macOS code signing for start)
set (optionalMembers codesignIdentity developmentTeamId)
set (returnAs codesignIdentity developmentTeamId)
# optional members (macOS code signing for start, autoupdate next)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the comment here and below for the language list.

winLangCharsetStr = '040904b0'
if languageCode != 'INT':
winLangCharsetStr = localizationMappingTable.get (languageCode, winLangCharsetStr)
projGenParams.append (f'-DAC_WIN_LANGCHARSET_STR={winLangCharsetStr}')
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it comes from the header, but I don't like this *_STR/*Str suffix. Nothing else has this, so let's keep it that way.

Comment on lines +152 to +156
if (autoupdate STREQUAL "1")
set (autoupdate "\n\t\t\tVALUE \"Autoupdate\", \"1\"")
else ()
set (autoupdate "")
endif ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (autoupdate STREQUAL "1")
set (autoupdate "\n\t\t\tVALUE \"Autoupdate\", \"1\"")
else ()
set (autoupdate "")
endif ()
set (autoupdate "")
if (autoupdate STREQUAL "1")
set (autoupdate "\n\t\t\tVALUE \"Autoupdate\", \"1\"")
endif ()

In the other places as well.

with open(gsLocalizationPath, 'r', encoding='utf-8') as f:
gsLocalizationContent = f.read ()

if not pattern:
Copy link
Contributor

Choose a reason for hiding this comment

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

Few lines above you already assert that the pattern is not None.

import re


def FillLocalizationMappingTable (devKitPath) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

This returns a dict[str, str], not a str. You can also type hint that the devKitPath is a Path (from pathlib).

Copy link
Author

Choose a reason for hiding this comment

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

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants