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

Add JSON UI and optional sort for metadata widget #877

Merged
merged 7 commits into from
Sep 2, 2020

Conversation

ajbozarth
Copy link
Member

@ajbozarth ajbozarth commented Aug 19, 2020

Fixes #851

Adds a default JSON viewer to the MetadataWidget and adds an optional sort which default to alphabetical by display name.

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@ajbozarth ajbozarth added the status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. label Aug 19, 2020
@ajbozarth ajbozarth self-assigned this Aug 19, 2020
@ajbozarth
Copy link
Member Author

Screen Shot 2020-08-18 at 5 03 25 PM

Screen Shot 2020-08-18 at 5 03 34 PM

Screen Shot 2020-08-18 at 5 04 00 PM

Screen Shot 2020-08-18 at 5 04 14 PM

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@ajbozarth ajbozarth removed the status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. label Aug 24, 2020
@ajbozarth
Copy link
Member Author

I moved the new react component into the ui-components package and cleaned it up a bit. This should be ready for review now

@ptitzler
Copy link
Member

I guess I'm missing something. The tab doesn't show up after I ran make clean install on your branch. Is it supposed to be there?

@ajbozarth
Copy link
Member Author

You have to open it via the Command Palette tab, it is not enabled by default

@ptitzler
Copy link
Member

  • Command/Ctrl Shift C
  • Elyra > Show Runtime Images

So I don't have to look it up again (:

@ptitzler
Copy link
Member

Very convenient to have as an interim solution for generic metadata!

Three thoughts (I realize the last two might be not specific to this PR but since you are already touching the metadata widget ...):

  • Is there a need to display the metadata node? If it doesn't serve an purpose and we would only ever display child properties of metadata (which seems to be the case for runtime images) there shouldn't be a need for it.
    image

  • Could the widget automatically sort the metadata entries by name? As the list of entries grows it gets harder and harder to find the entry ...
    image

  • We distinguish between user-defined and system-defined metadata. In above screen capture only the last one is user-defined metadata. As a user I can only delete user-defined metadata. Trying to delete a system-defined metadata an error is raised. Could we hide the delete icon for system-defined entries that cannot be deleted?

@ajbozarth
Copy link
Member Author

  • Is there a need to display the metadata node? If it doesn't serve an purpose and we would only ever display child properties of metadata (which seems to be the case for runtime images) there shouldn't be a need for it.

This was a side effect of how the JSON Component was written. I can play with it some and see if I can show only the contents of a node without showing a parent. (should be possible)

  • Could the widget automatically sort the metadata entries by name? As the list of entries grows it gets harder and harder to find the entry ...

Actually having an optional sort that defaults to alpha when enable and can be overridden would be a good feature, I can tack it in here if it seems straight forward to add (otherwise I'll open another issue and pr to do it).

We distinguish between user-defined and system-defined metadata. In above screen capture only the last one is user-defined metadata. As a user I can only delete user-defined metadata. Trying to delete a system-defined metadata an error is raised. Could we hide the delete icon for system-defined entries that cannot be deleted?

I actually discussed this some with @kevin-bates a while ago and currently the api doesn't distinguish whether metadata is user-defined or system-defined until you call the delete api. I believe a "fix" for this on either the front or back ends would be overly complicated and messy for little benefit. Especially because you can "edit" a default to override it, at which point you could delete that edited version.

@kevin-bates
Copy link
Member

We distinguish between user-defined and system-defined metadata. In above screen capture only the last one is user-defined metadata. As a user I can only delete user-defined metadata. Trying to delete a system-defined metadata an error is raised. Could we hide the delete icon for system-defined entries that cannot be deleted?

Only the server distinguishes between user and system-define metadata instances. We would need to add a meta-property to indicate this - probably something like a boolean-valued system_owned would be sufficient.

However, because we allow system-owned instances to be modified and essentially become user-defined instances - which can be deleted - then things get very confusing. For example, if we were to set the system_owned meta-property to False when a user "updates" a system-owned instance, then the UI would show a delete icon and allow its deletion. But, upon its deletion, the actual system-owned instance - of the same name - reappears and, this time, without a delete icon.

As a result, I'd be inclined to a) add a system-owned meta-property where necessary (i.e., schemas that can span the user/system domain), and b) disallow updates to system-owned instances. This would mean that a user wishing to "update" a system-owned instance would be required to create a new instance with a different name containing the updated properties.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@ptitzler
Copy link
Member

then things get very confusing.

Yep, I noticed that:

  • edit a system-defined metadata entry (OK)
  • delete the same entry (which is now a user-defined entry, OK)
  • but an entry is still visible, which is (again) a system-defined, which cannot be deleted (FAIL)

This would mean that a user wishing to "update" a system-owned instance would be required to create a new instance with a different name containing the updated properties.

For the sake of simplicity that's the option I would favor, even though a case could be made to not restrict editing/deletion of system-defined metadata at all. (Ignoring for now that a user can already do that by manually accessing the file system with the appropriate permissions.)

@ajbozarth
Copy link
Member Author

I pushed a couple commits addressing @ptitzler commit's. I updated the display to not show the "root" node and just the json contents. I also added an optional default alphabetical sorting based on display_name for MetadataWidget, which I disabled in CodeSnippetWidget but enabled for RuntimesWidget. Is alphabetical sorting something we would like for the Runtimes tab? Would we want to add it to Code Snippets?

New screenshot:
Screen Shot 2020-08-24 at 4 23 12 PM

@ajbozarth ajbozarth changed the title Add JSON UI for metadata widget Add JSON UI and optional sort for metadata widget Aug 24, 2020
@ptitzler
Copy link
Member

ptitzler commented Aug 25, 2020

Thank you Alex!

I do believe all lists should be sorted by default using a criteria that enables the user to quickly find relevant information for the majority of use case scenarios. A system-sorted order (whatever that means specifically as to how the sort order is implemented explicitly) shouldn't be used unless it provides a clear advantage. Let's assume (dunno if that is the case) the current system-sort order for the metadata widgets is "by change date [of the underlying file], descending". Does the user derive any benefits from having the list presented in that order? How would the criteria help the user find the appropriate entry faster compared to an alphabetically sorted list?

Example: You are looking for an entry uvw
You see a partial list and need to scroll/page down multiple times to see all the entries.

  • abc
  • def
  • ghi

Most likely you would assume that this list is ordered alphabetically (given the implicit hint) and jump to the bottom of the list instead (or close to it) instead of moving lower just a little bit to view the next couple entries. With system sorted order you can't do that without having explicit knowledge about that order. You also have to know the "actual sort value" or at least how it compares to the value of other entries. ("The runtime image definition I am looking for was changed 12 days ago, but where does that put the entry in the list if I don't know when other definitions were last updated?")

@ptitzler
Copy link
Member

ptitzler commented Aug 25, 2020

Not sure why but when you create a metadata entry or open an existing one for editing the first input field is no longer selected by default. I remember you had this fixed in another PR ...
Other than that this PR works for me as expected.

@kevin-bates
Copy link
Member

Regarding sorting, I agree that we should have a consistent message regarding sorting - with the default being that items are sorted by display_name. Code snippets, however, should be grouped by language, then sorted by display_name, with the groupings sorted by language.

I don't know what the implementation of the Code Snippets list widget (not sure that's the correct term) is, but we could probably introduce a meta-property to the schemas named something like group_by. This would be an optional string-valued meta-property (i.e., resides outside the metadata stanza). For Code Snippets, it would have a value of language. Applications processing these instances would know to first group Code Snippets instances by language, then sort them by display_name within a grouping. Schemas that do not use group_by, always sort by display_name.

The same rules should apply to the CLI tooling as well.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@ajbozarth
Copy link
Member Author

Based on the conversation above I updated to also sort code snippets using the method Kevin described

Copy link
Member

@karlaspuldaro karlaspuldaro left a comment

Choose a reason for hiding this comment

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

Tested this locally and looks good :)

Copy link
Member

@marthacryan marthacryan left a comment

Choose a reason for hiding this comment

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

Just tested locally and LGTM!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
* A React Component for displaying a json object
*
* A slimmed down copy of the `Component` class in @jupyterlab/json-extension
*/
Copy link
Member

Choose a reason for hiding this comment

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

I have updated the license to reflect the info above.

Copy link
Member

@lresende lresende left a comment

Choose a reason for hiding this comment

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

LGTM

@lresende lresende merged commit 10ef9b9 into elyra-ai:master Sep 2, 2020
@ajbozarth ajbozarth deleted the json branch October 12, 2020 20:38
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.

Runtime image editor: present metadata in a user-friendly way
6 participants