Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NUI] Prepare extra space to View to attach objects from external #6494

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

rabbitfor
Copy link
Collaborator

Description of Change

View provides methods to attach extra data (that is not bindable property) from the outside.

API Changes

  • ACR:

Copy link
Contributor

@jaehyun0cho jaehyun0cho left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -48,6 +48,7 @@ public partial class View : Container, IResourcesProvider
private LayoutTransition layoutTransition;
private TransitionOptions transitionOptions = null;
private ThemeData themeData;
private Dictionary<Type, object> attached;
Copy link
Contributor

Choose a reason for hiding this comment

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

정확히 테스트 해보고 말씀 드리는것은 아닙니다만, 혹시라도 문제가 있을까 해서 문의 드립니다.
attached 되는 view 가 dali binding 된 객체이기 때문에 정상적이면 GC 되면서 attached 가 unreachable 되겠지만, dispose queue 에 머물러 있는 상태면 reference 가 보존되어 attached 된 객체들이 GC 가 안 될 것 같은데요, 혹시 Dispose(bool disposing) 에서 강제로 null 로 만들어서 이 코너케이스를 회피할 수 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Explicit dispose 케이스에 대해서 attached를 null로 날려버리는 코드를 추가했습니다.
다만 말씀하신 dispose queue에 들어가는 케이스는.. 제가 이해한 것이 맞는지 모르겠지만 이 시점은 이미 view가 GC의 대상으로 선정된 상태 (implicit dispose) 인데 이 케이스에서는 managed data에 접근하면 안되는 것으로 알고 있어서 (그 시점에 이미 멤버가 GC 되었을 수 있음) attached를 어떻게 하지는 못할 것 같습니다.

@rabbitfor rabbitfor merged commit cfdbcf4 into Samsung:DevelNUI Dec 6, 2024
3 checks passed
@rabbitfor rabbitfor deleted the attached branch December 6, 2024 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants