Implement path for in-app guides

[edan-bainglass]

Based on #967

This PR adds a mechanism for adding in-app guides meant as example use-cases for users. The mechanism leverages a global instance of the GuideManager, which tracks the current selected guide in an active_guide traitlet, which is observed by all InAppGuide instances. Instances are placed throughout the app with unique identifiers. Additional placeholders may be added by plugin developers within their defined panels.

Usage

In-app guide widgets can be added anywhere as follows:

from aiidalab_qe.common.infobox import InAppGuide

example = InAppGuide(
    children=[
        ipw.HTML("An example in-app guide."),
    ],
)

This defines the guide explicitly, providing hard-coded content, which is useful for generic guide sections that are useful for any guide. Children can be any widget supported by ipywidgets. This allows developers to customize the content of the guide as they wish.

However, for specific guide content, a more flexible approach is

from aiidalab_qe.common.infobox import InAppGuide

example = InAppGuide(identifier="structure-step")

Here, the guide is defined from content fetched by an HTML id attribute from the currently loaded guide (HTML document) in the guide manager corresponding to the active (selected) guide. The HTML document is parsed using beautiful soup.

In either case, CSS classes can be provided to the InAppGuide constructor to override the behavior of the info-box and in-app-guide CSS classes.

example = InAppGuide(
    identifier="structure-step",
    classes=["custom1", "custom2", etc.],
)

This allows developers to control top-level styles for the guide. Styles internal to the guide are more easily controlled directly in the guide HTML documents.

External guides

The app defines internal guides in src/aiidalab_qe/guides. External guides may be added by plugin developers using the same entry point system already used in plugins. Developers simply add a guides key to their plugin definition with a value pointing at the path of their guides directory. The guide manager will pick these up on app start and add them to the list of available guides.

Comments regarding external guides

Entry point

We may consider enforcing a structure here, specifying that external guides must be provided at the root level of the plugin.

• Pro: developers don't need to bother with adding the new guides key
• Con: rigid placement and slightly more complex approach to finding the guides directory (easy if there is a simple means of obtaining the path of the plugin directory)

Uniqueness

In the current implementation, there is a risk an external guide developer names a guide identically to an existing internal/external guide. To avoid this, the plugin identifier can be used as a prefix when storing guides in the manager. In the app, we could consider displaying guides by category. There are various means of achieving this cleanly. Open to discussion.

[codecov]

Codecov Report

Attention: Patch coverage is 86.73469% with 13 lines in your changes missing coverage. Please review.

Project coverage is 68.53%. Comparing base (5949bdf) to head (bfa4b02).

Files with missing lines	Patch %	Lines
src/aiidalab_qe/app/wrapper.py	69.23%	8 Missing ⚠️
src/aiidalab_qe/common/guide_manager.py	90.24%	4 Missing ⚠️
src/aiidalab_qe/common/infobox.py	95.45%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #763      +/-   ##
==========================================
+ Coverage   68.26%   68.53%   +0.26%     
==========================================
  Files         110      111       +1     
  Lines        6211     6302      +91     
==========================================
+ Hits         4240     4319      +79     
- Misses       1971     1983      +12     

Flag	Coverage Δ
python-3.11	68.53% <86.73%> (+0.26%)	⬆️
python-3.9	68.55% <86.73%> (+0.26%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[superstar54]

Hi @edan-bainglass , is this PR ready for review? I like the design, and I am looking forward to having it in the APP.

[edan-bainglass]

@superstar54 it is :) The failed tests are due to a chain of yet-to-be-merged PRs all the way back to aiidalab/aiidalab-widgets-base#624, which @danielhollas is reviewing.

[superstar54]

When reviewing this PR, do I need to switch aiidalab/aiidalab-widgets-base#624?.

[edan-bainglass]

If you're testing locally, then yes.

[edan-bainglass]

Need to implement selenium tests to make sure the widget works properly

[edan-bainglass]

@superstar54 @danielhollas rebased on top current state. Please give it a quick look. @superstar54 let me know if I can remove the blocked label.

[danielhollas]

Just a super quick thoughts.

Thanks on working on this @edan-bainglass looks super intriguing. Unfortunately this week is super busy and on Friday I am leaving for two week holiday. But I am looking forward to delving into this work once I get back.

[danielhollas]

Is quide_id currently used anywhere? What is its purpose?

The name of this is confusing, since below it is added to extra classes, so it's not really an id in a CSS sense.

Also, how is the uniqueness enforced. Is it up to the programmer to ensure that these ids are unique in the whole app? Not sure if that is scalable (especially given that there are also external plugins that supposedly could use this as well perhaps?)

[edan-bainglass]

The guide id class is used by JS code to toggle the guide. Suppose you were making a new guide for bands. You'd add these widgets all around with the guide id "bands". You would then add a new guide item in the guides list with the key "bands". Selecting the guide bands will toggle all InAppGuides with the bands id.

Now, regarding collisions with existing classes, I'd have to think about it. If someone introduced the bands class, it might override, or unintentionally add styles. Will think and let you know.

[edan-bainglass]

Naively, I'd enforce that the guide id to be of a certain "unique" format, but I suppose one could always think of a case where that may be accidently used. Not sure how realistic that assumption is. Happy to consider other suggestions.

[edan-bainglass]

@danielhollas back to this PR. Unfortunately, I am not sure at the moment how to handle this. It is actually not an id, so I'll be changing it to guide_class. It needs to be a class (and not an id), because multiple widgets will share this guide class, such that they can be toggled in unison. I think it is okay for now.

As for uniqueness, I can maybe append -guide to the guide class, so for example bands-guide-identifier. This should help a bit. I can make it more unique of course, e.g. this-is-a-super-unique-guide-class-identifier-for-bands.

[danielhollas]

It is actually not an id, so I'll be changing it to guide_class

Makes sense 👍

As for uniqueness, I can maybe append -guide to the guide class, so for example bands-guide-identifier.
I like the idea to append a string to make this a bit more unique to prevent random clashes in the DOM from outside this widget. Perhaps appending -in-app-guide? That's less generic than just "guide" and is consistent with the classname that you're already adding.

However, my concern was more with how to ensure uniqueness within this class itself, e.g. how to prevent programmer from e.g. copy-pasting a snippet that creates an instance of InAppGuide and forgetting to change the guide_class? QeApp is large enough that one could imagine that making sure all instances of this widget have unique name is non-trivial (not to mention if we ultimately choose to move this widget to AWB).

Perhaps for this you could create a class variable to would hold a list of these guide_class identifiers across all instances, and we could than error out in the __init__ if we detect a duplicate? Something like

class InAppGuide(InfoBox):
    unique_class_names: set = {}
    
    def __init__(self, guide_class: str, classes: list[str] | None = None, **kwargs):
        if guide_class in unique_class_names:
            raise ValueError()
        unique_class_names.add(guide_class)

(just an idea, perhaps I am overthinking it)

[edan-bainglass]

That's interesting. Note that after some discussion, we've decided to pursue a plugin-based approach for these. So a guide for bands will be provided by the bands plugin (along its settings, results, codes, etc.). The guide radio buttons would then be autodetected from entry points. The guide itself would be limited to the components of the plugin. So for bands, the settings panel and results panel(s).

That said, uniqueness should be handled carefully. Will consider your comment 👍

[superstar54]

Hi @edan-bainglass , thanks for the nice work!

Could you add a section on how to add in-app guide in the docs?

[edan-bainglass]

@superstar54 will do 👍

[superstar54]

Very nice in-app guide! The style is also pretty. I added a few suggestions in the comments.

Could you also add a guide for step 4.

[superstar54]

Suggested change

	<li>Select resources</li>
	<li>Select resources in the Global resources tab</li>

[superstar54]

Since "Full geometry" is the default, consider select "Atomic positions"

[superstar54]

Similar to the advacned settings,

Suggested change

	<div class="alert alert-success">???</div>
	<div class="alert alert-success">In this walkthrough, we will not modify the pdos settings and proceed with the defaults.</div>

[superstar54]

Maybe also add a description of the calculation before submitting.

[edan-bainglass]

Very nice in-app guide! The style is also pretty. I added a few suggestions in the comments.

Could you also add a guide for step 4.

Thanks @superstar54. Note that I will make no comments regarding the actual content of the guides, as the current content is only a draft to give @giovannipizzi and @cpignedoli a sense of how it works. I will work with them in an immediately following PR to iron out the actual content, at which point, we will also cover step 4 🙂

Since all your comments above ARE regarding content, I take it you approve of the mechanism itself. Per our discussion, I will update the PR to handle uniqueness as suggested in the PR description.

[danielhollas]

Per our discussion, I will update the PR to handle uniqueness as suggested in the PR description.

LMK when you're done with that and I can have a final look (but feel free to merge anytime / don't feel block by me)

[edan-bainglass]

@danielhollas ready to review. I implemented guide uniqueness in the last commit. Also in that commit is a mechanism for ensuring intended guide order by inspecting the HTML file name, which is now required to include a #_ prefix to indicate the intended order, which I parse. I did this sort of quick, so if you think of a better approach, do share 🙂