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

More flexibility possible? #1368

Open
StephanSchuster opened this issue Jul 12, 2024 · 12 comments
Open

More flexibility possible? #1368

StephanSchuster opened this issue Jul 12, 2024 · 12 comments

Comments

@StephanSchuster
Copy link

I just started evaluating ADC. Really cool and useful. Thanks for all your work so far.

Do you think the following two features would be possible in future releases of ADC in order to increase its flexibility:

  1. Load offline docs also from resources (not only assets) --> enables Runtime Resource Overlays (RRO)
  2. Support dynamic docId and rootNode (e.g. via params) instead of generating/hardcoding them --> enables custom loading logic

Any feedback appreciated.

More details on (1):
As far as I can see, this is where you currently try to load offline *.dcf files from the assets/figma dir:

Before doing so, you could check in res/raw/ and then fall back to assets/figma. In consequence we could apply new *.dcf files via RRO which would be a great and mighty new feature.

More details on (2):
Currently code like this

@DesignDoc(id = "XYZ...")
interface Test {

    @DesignComponent(node = "#MyRootNode")
    fun Main(
        ...
    )
}

results in the following generated code:

@Composable
final fun Main(...) {
    ...
    var nodeName = "#MyRootNode"
    ...
    val (docId, setDocId) = remember { mutableStateOf(DesignDocId("XYZ...", "")) }
    ....
}

See:

" val (docId, setDocId) = remember { mutableStateOf(DesignDocId(\"$docId\", \"$designVersion\")) }\n"

I think if we could provide docId and rootNode dynamically, e.g. via optional params to the generated Composable, this would provide a hook for custom loading mechanisms. I other words, I would wish for a way to load a dedicated Figma doc/branch like the DesignSwitcher but from my own code.

@StephanSchuster
Copy link
Author

StephanSchuster commented Jul 15, 2024

Regarding (2): I guess I should simply not generate code via annotations but use the DesignDoc and CustomizationContext directly to achieve the requested dynamic/flexibility. In other words: write the generated code myself with the appropriate adaptions. Correct?

@rylin8
Copy link
Collaborator

rylin8 commented Jul 17, 2024

You can change the doc ID with the DesignDocOverride function. Example:

I'm not sure what your goal is exactly for changing the rootNode, but you can create different functions with different root nodes and call the appropriate one at runtime.

@rylin8 rylin8 moved this to Needs More Info in DesignCompose Sep 9, 2024
@rylin8
Copy link
Collaborator

rylin8 commented Sep 9, 2024

@StephanSchuster please let us know if the DesignDocOverride() function is sufficient.

@StephanSchuster
Copy link
Author

Hi @rylin8, thanks for catching up and sorry that I did not reply since my initial question. I haven't had time since then to work with ADC. Now this hopefully changes.

You mentioned DesignDocOverride(). That's good to know, but not what I need. My overall goal is to have more control over the logic what to load from where and how to customize it - at runtime, dynamically, not via annotations. In my initial question I therefore asked for two related features.

I think for (2) I will be good to proceed as I mentioned in my second comment.

And for (1) I was hoping that you could add something like this into DocServer:

// ...
val assetManager = context.assets
try {
    @Suppress("DiscouragedApi")
    val resourceId = context.resources.getIdentifier("figma_${id.lowercase()}_dcf", "raw", context.packageName)
    val assetDoc = if (resourceId != 0) {
        // If file exists in res/raw, load from there
        context.resources.openRawResource(resourceId)
    } else {
        // Otherwise, load from assets (like done now)
        assetManager.open("figma/$id.dcf")
    }
    val decodedDoc = decodeDiskDoc(assetDoc, null, docId, Feedback)
    // ...
}
// ...

Background: files in assets cannot be overlayed via Runtime Resource Overlays. Only files in res can. With this I could provide a new Figma DCF file bundeled in a RRO at runtime and the app would change its appearance.

Maybe the order should be the other way round: assets first, and only if not found, check res. With that, overlaying would be prevented if a file in assets is provided.

Do you think there is a chance that you will add such functionality?

@timothyfroehlich
Copy link
Member

@StephanSchuster we actually did earlier this week! #1778. We haven't added documentation for it, but the test that was added should show how to use it. It will be released as part of 0.32, and we cut 0.32.0-rc01 the other day.

@StephanSchuster
Copy link
Author

StephanSchuster commented Nov 15, 2024

@timothyfroehlich, @dipenpradhan : Yes, yes, yes!!! This is awesome. Thank you so much. I think this is a great addition.

I just tested it in rc-01. Works! One thing, however ...

The current comment for setRawResourceId(@RawRes resourceId: Int) says "Live updates do not work when this is set.". From what I see in code and from what I saw in my tests, this is not true. Live updates DO work and have priority.

The priorities as far as I can tell are:

  1. Live updates from Figma (if DesignSettings.enableLiveUpdates(this) is set)
  2. Offline file from res/raw (if DesignSettings.setRawResourceId(R.raw.xyz) is set)
  3. Offline file from assets
  4. Downloaded/cached files from app's filesDir

For me, this totally makes sense. Can you confirm that the above is true and you will stick with this?

@StephanSchuster
Copy link
Author

Along the idea of "more flexibility", would it be possible to have a disableLiveUpdates()?

Background: We want/need to toggle various DesignSettings at runtime or rather call them with different values. While this works for setRawResourceId() and more or less for addFontFamily() (a "clear" method would be nice), we currently can only enableLiveUpdates() but never switch it off again.

@rylin8
Copy link
Collaborator

rylin8 commented Nov 18, 2024

The comment regarding live update not working is a bit misleading. If you enable live update the design switcher will show errors and you likely won't be able to see your app. We are aware of this issue and filed #1802 to fix it.

@StephanSchuster
Copy link
Author

@rylin8 : I think the current logic/priorities/implementation is fine as it is (see 1.-4. above). It totally makes sense. Also the single res/raw. I did not experience any errors. I would not change anything. I would just fix the comment.

@StephanSchuster
Copy link
Author

Thanks for releasing 0.32.0 including the new resource loading. This is an important first step for how we plan to use ADC in the future. And while I know that this is only the first shot, I have some more comments, questions and findings (which you might already be aware of).

Assume the following:

  • My app contains a res/raw/figma.dcf file which can be overlayed via RRO
  • My app loads/uses different nodes from Figma based on a config file which is applied and processed at runtime
  • RRO 1 includes its own res/raw/figma.dcf file which contains only node #A plus a config file that results in using #A in code/UI
  • RRO 2 includes its own res/raw/figma.dcf file which contains only node #B plus a config file that results in using #B in code/UI
  • No files are currently cached in my app's filesDir nor does it have an *.dcf files in assets

Based on the above:

  • RRO 1 is applied to my app --> #A is shown as configured and everything works as expected
  • While the app is still running RRO 2 is applied (it does not matter if RRO 1 gets disabled first)
  • Result: #B is not found because the old design doc with only #A was cached in memory and gets reused
  • If at this point I would restart my app, memory gets cleared and I would see #B of the active RRO 2
  • ADC caches docs based on docId which I think is good
  • RRO 1 and RRO 2 use the same docId which I think should be valid/supported as well

Now that I am writing this, I am not 100% sure anymore if the same docId results from the dynamic way how we are using ADC or if this happens with static code as well. I could look it up, but I guess you know it by heart and can judge for yourself if this is a situation that you want to handle.

The way I am currently handling this is by clearing the ADC cache similar to your ClearStateTestRule whenever I notice that the MD5 hash of the res/raw/figma.dcf changed. In fact, I do this:

fun clearCache(context: Context, figmaConfig: FigmaConfig) {
    // Delete cache file to avoid outdated artifacts
    deleteCacheFile(context, figmaConfig)
    // Imitate "ClearStateTestRule" from ADC codebase
    clearStates()
    clearDocuments()
    clearFileFetchStatus()
    DesignSettings.setRawResourceId(Resources.ID_NULL)
    // Additional stuff that seems required or useful
    clearSubscriptions()
    clearFirstFetch()
}

Each method is currently using reflection. Yes, ugly, super ugly, but the only option I have as of now. Is there a chance that you offer a public API method to clear the cache in some way or the other? This might help others as well. Besides the scenario mentioned above, we also need to clear the ADC cache whenever we notice that because of a new configuration the list of needed Figma root nodes ("server params") changed. In that case we would also face the above "node not found" situation. Hence, we clear the cache and download a new doc with all needed root nodes.

Okay, this now gets a bit wired, sorry. I guess in the end it boils down to: can we have a official clearCache() method?

One last related thing: just skimmed again over the DocServer code. If I have setRawResource() and no live updates enabled (we also do this dynamically) but some cached files on filesDir, the latter take prescedence over the raw/res/...dcf. Correct? Is this a bug or a feature? Could in fact be both. If I am right, I am not sure which approach I would prefer. Just wanted to ask.

@rylin8
Copy link
Collaborator

rylin8 commented Nov 27, 2024

First, the setRawResource function now takes a Figma document ID as a parameter so that multiple documents can be set to load from different raw resources. This change will be available in 0.33.

Second, the load order is unfortunately a bit confusing at the moment. On app startup, it should load from assets/raw resources if it exists, at which point the doc is added to a memory cache. After that, the document will always be found in the cache. I realize this is not ideal for supporting RRO and changing the resource. This is a new feature that hasn't been full worked out yet. We will discuss this issue soon, I've filed #1814 to track the issue. Thanks for the input!

@StephanSchuster
Copy link
Author

@rylin8 : Thanks for your answer. We are looking forward to see this feature evolving. In case you want more input, also e.g. on our "dynamic usage" of ADC, we (@Elektrobit) would be happy to schedule a call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs More Info
Development

No branches or pull requests

3 participants