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 edges and links to table view #2011

Open
wants to merge 43 commits into
base: master
Choose a base branch
from

Conversation

Quasar985
Copy link
Collaborator

Prerequisites

  • Reviewed the checklist

  • Reviewed feedback from the "Sonar Cloud" bot. Note that you have to wait
    for the "CI / Unit Tests") to complete first. Failed Unit tests can be
    debugged by adding the label "verbose logging" to the GitHub PR.

Description of the Change

Added functionality to view edges and links in the table view, as well as the existing transactions are vertices.

Alternate Designs

For selecting edges/links on the graph and have that selection be reflected in the table view:
This was achieved iterating over the selected transactions on the graph and selecting edges/links in the table view that those transactions represent. An alternate design would be to add functionality to the selection coed that handles edges/links and use that information directly.

Why Should This Be In Core?

Benefits

Users can now view information about edges and links in table view.

Possible Drawbacks

Aside from the usual risk of changing code, none.

Verification Process

Verified visually and through automated testing.

Applicable Issues

#1859

Copy link
Collaborator

@antares1470 antares1470 left a comment

Choose a reason for hiding this comment

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

  1. If you could add a whatsnew entry for this, that would be great
  2. Would also be good if you could update the table view help page to include this change
  3. From testing, I'm not exactly sure what the intention is to display, but it seems at the moment that if you have more than one value for an attribute in an edge/link (which you might encounter if they consist of more than one transaction), it doesn't display all the values (see the image below)
  4. Also from testing, perhaps if the menu list order is modified slightly so that Vertex and Transaction are switched around, that would be good to group Transaction, Edge, and Link together
    tableLinkEdgeIssue

@antares1470
Copy link
Collaborator

antares1470 commented Apr 16, 2024

3. From testing, I'm not exactly sure what the intention is to display, but it seems at the moment that if you have more than one value for an attribute in an edge/link (which you might encounter if they consist of more than one transaction), it doesn't display all the values (see the image below)

I would note from this that we would likely want to cap the values somewhere (if an edge consists of 50 transactions and each has a unique value for an attribute, you probably don't want to blow out the table with 50 entries in the one cell, maybe)

@Quasar985
Copy link
Collaborator Author

  1. From testing, I'm not exactly sure what the intention is to display, but it seems at the moment that if you have more than one value for an attribute in an edge/link (which you might encounter if they consist of more than one transaction), it doesn't display all the values (see the image below)

I would note from this that we would likely want to cap the values somewhere (if an edge consists of 50 transactions and each has a unique value for an attribute, you probably don't want to blow out the table with 50 entries in the one cell, maybe)

I'll look into that.

I also just need some further clarification, because I'm not quite sure what you mean when you say it doesn't show all values for an attribute of something like an edge. In your screenshot it shows that the two types for the transactions in the highlighted edge are 'unknown' and 'correlation'. I can see one of those in the table being highlighted, and that has a transaction.type of 'unknown' so it must be partially correct from what I can see. Or do you mean something else isn't displaying like it normally should?

@Quasar985
Copy link
Collaborator Author

  1. From testing, I'm not exactly sure what the intention is to display, but it seems at the moment that if you have more than one value for an attribute in an edge/link (which you might encounter if they consist of more than one transaction), it doesn't display all the values (see the image below)

I would note from this that we would likely want to cap the values somewhere (if an edge consists of 50 transactions and each has a unique value for an attribute, you probably don't want to blow out the table with 50 entries in the one cell, maybe)

I'll look into that.

I also just need some further clarification, because I'm not quite sure what you mean when you say it doesn't show all values for an attribute of something like an edge. In your screenshot it shows that the two types for the transactions in the highlighted edge are 'unknown' and 'correlation'. I can see one of those in the table being highlighted, and that has a transaction.type of 'unknown' so it must be partially correct from what I can see. Or do you mean something else isn't displaying like it normally should?

Wait, I think I understand the problem now. The row in the table view doesn't display the correct data for multiple transactions.

@Delphinus8821 Delphinus8821 self-requested a review April 16, 2024 05:53
@antares1470
Copy link
Collaborator

Wait, I think I understand the problem now. The row in the table view doesn't display the correct data for multiple transactions.

Yes. An edge (and link) can consist of multiple transactions and therefore the edge/link could contain multiple values for a given attribute and so this should probably be reflected in the table view. I'm not exactly sure of the best approach though given as highlighted, too many values may blow out the table more than desired

@Quasar985 Quasar985 self-assigned this Apr 16, 2024
@Quasar985
Copy link
Collaborator Author

Yes. An edge (and link) can consist of multiple transactions and therefore the edge/link could contain multiple values for a given attribute and so this should probably be reflected in the table view. I'm not exactly sure of the best approach though given as highlighted, too many values may blow out the table more than desired

I'm thinking the table view should display <Multiple Values> if that attribute is not the same for all transactions, similar to the attribute editor. If the user wants to view all the different values, they could also use the attribute editor. What do you think?

@Quasar985 Quasar985 requested a review from antares1470 April 18, 2024 00:55
@Quasar985 Quasar985 requested a review from OrionsGuardian May 7, 2024 01:05
Copy link

sonarqubecloud bot commented May 7, 2024

Copy link

Copy link

This pull request is stale because it has been open for 6 months with no activity. Consider reviewing and taking an action on this pull request.

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