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

data-menu: implement action bar (delete, edit subset, metadata) #3264

Merged
merged 20 commits into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
New Features
------------

* New design for viewer legend. [#3220, #3254, #3263]
* New design for viewer legend. [#3220, #3254, #3263, #3264]

Cubeviz
^^^^^^^
Expand Down
8 changes: 6 additions & 2 deletions jdaviz/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ def to_unit(self, data, cid, values, original_units, target_units):
'j-plugin-popout': 'components/plugin_popout.vue',
'j-multiselect-toggle': 'components/multiselect_toggle.vue',
'j-subset-icon': 'components/subset_icon.vue',
'j-plugin-live-results-icon': 'components/plugin_live_results_icon.vue',
'j-child-layer-icon': 'components/child_layer_icon.vue',
'plugin-previews-temp-disabled': 'components/plugin_previews_temp_disabled.vue', # noqa
'plugin-table': 'components/plugin_table.vue',
'plugin-dataset-select': 'components/plugin_dataset_select.vue',
Expand All @@ -152,7 +154,9 @@ def to_unit(self, data, cid, values, original_units, target_units):
'plugin-color-picker': 'components/plugin_color_picker.vue',
'plugin-input-header': 'components/plugin_input_header.vue',
'glue-state-sync-wrapper': 'components/glue_state_sync_wrapper.vue',
'data-menu-add-data': 'components/data_menu_add_data.vue'}
'data-menu-add-data': 'components/data_menu_add_data.vue',
'data-menu-remove': 'components/data_menu_remove.vue',
'data-menu-subset-edit': 'components/data_menu_subset_edit.vue'}

_verbosity_levels = ('debug', 'info', 'warning', 'error')

Expand Down Expand Up @@ -2287,7 +2291,7 @@ def vue_data_item_remove(self, event):
data_label = event['item_name']
data = self.data_collection[data_label]
orientation_plugin = self._jdaviz_helper.plugins.get("Orientation")
if orientation_plugin is not None:
if orientation_plugin is not None and orientation_plugin.align_by == "WCS":
Comment on lines -2290 to +2294
Copy link
Member Author

Choose a reason for hiding this comment

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

@bmorris3 - can you confirm if this is correct? I ran into this bug when adding test coverage (where it would complain otherwise that data_label == '' was not available as new_parent). Do you think this should be split out to a separate PR and backported?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I actually just encountered this yesterday too! I was experimenting with @eteq's unloading-data questions and was trying to untangle the bug. It looks like this is probably what we need. Testing this out now.

from jdaviz.configs.imviz.plugins.orientation.orientation import base_wcs_layer_label
orient = orientation_plugin.orientation.selected
if orient == data_label:
Expand Down
14 changes: 14 additions & 0 deletions jdaviz/components/child_layer_icon.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<template>
<j-tooltip
:tooltipcontent="'sublayer of '+icon[0].toUpperCase()"
span_style="display: inline-block"
>
<v-icon dense>mdi-layers-outline</v-icon>
</j-tooltip>
</template>

<script>
module.exports = {
props: ['icon']
};
</script>
61 changes: 61 additions & 0 deletions jdaviz/components/data_menu_remove.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
<template>
<v-menu
absolute
offset-y
left
>
<template v-slot:activator="{ on, attrs }">
<j-tooltip
:span_style="'display: inline-block; float: right; ' + (delete_enabled ? '' : 'cursor: default;')"
:tooltipcontent="delete_tooltip"
>
<v-btn
icon
v-bind="attrs"
v-on="on"
:disabled="!delete_enabled"
>
<v-icon class="invert-if-dark">mdi-delete</v-icon>
</v-btn>
</j-tooltip>
</template>
<v-list dense style="width: 200px">
<v-list-item>
<v-list-item-content>
<j-tooltip
:tooltipcontent="delete_viewer_tooltip"
>
<span
style="cursor: pointer; width: 100%"
@click="() => {$emit('remove-from-viewer')}"
>
Remove from viewer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very nit-picky on the phrasing, but it bothers me a little bit that for data, this actually removes the data from the viewer, but with a subset, it just makes it invisible. Could we have conditional text here to reflect what is actually happening in the subset case? I know that's extra complicated since you could select both a subset and a data layer, this isn't a deal breaker for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, agreed. We could handle this with the text and/or tooltip changing based on the selection or we could disable it entirely if a subset is selected. @Jenneh - thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rosteen what do you think of updating the tooltip for subsets to say "hide from viewer" rather than "remove selected data"? would that be enough to clarify?

Copy link
Member Author

@kecnry kecnry Nov 7, 2024

Choose a reason for hiding this comment

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

there could also be three versions of the tooltip text that change based on the selection:
data only: "Remove selected data from this viewer only"
subsets only: "Hide selected subsets in this viewer"
data+subsets: "Remove selected data and hide selected subsets in this viewer"

(but I'd vote to maybe keep the button text fixed since it maps to the API method)

Copy link
Member Author

Choose a reason for hiding this comment

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

how is this?

Screen.Recording.2024-11-07.at.1.05.11.PM.mov

Copy link
Collaborator

Choose a reason for hiding this comment

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

That looks good to me!

</span>
</j-tooltip>
</v-list-item-content>
</v-list-item>
<v-list-item>
<v-list-item-content>
<j-tooltip
:span_style="'display: inline-block; float: right; ' + (delete_app_enabled ? '' : 'cursor: default;')"
:tooltipcontent="delete_app_tooltip"
>
<span
:style="'width: 100%; ' + (delete_app_enabled ? 'cursor: pointer;' : '')"
:disabled="!delete_app_enabled"
@click="() => {if (delete_app_enabled) {$emit('remove-from-app')}}"
>
Remove from app
</span>
</j-tooltip>
</v-list-item-content>
</v-list-item>
</v-list>
</v-menu>
</template>

<script>
module.exports = {
props: ['delete_enabled', 'delete_tooltip', 'delete_viewer_tooltip', 'delete_app_enabled', 'delete_app_tooltip'],
};
</script>
98 changes: 98 additions & 0 deletions jdaviz/components/data_menu_subset_edit.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
<template>
<v-menu
v-if="selected_n_subsets > 0"
absolute
offset-y
bottom
left
>
<template v-slot:activator="{ on, attrs }">
<j-tooltip
:span_style="'display: inline-block; float: right; ' + (subset_edit_enabled ? '' : 'cursor: default;')"
:tooltipcontent="subset_edit_tooltip"
>
<v-btn
text
class="invert-if-dark"
v-bind="attrs"
v-on="on"
:disabled="!subset_edit_enabled"
>
Edit Subset
</v-btn>
</j-tooltip>
</template>
<v-list dense style="width: 300px">
<v-list-item>
<v-list-item-content>
<j-tooltip :tooltipcontent="'Open '+subset_selected+' in Subset Tools plugin'">
<span
style="cursor: pointer; width: 100%"
@click="() => {$emit('view-info')}"
>
Edit in plugin
</span>
</j-tooltip>
</v-list-item-content>
</v-list-item>

<v-divider></v-divider>

<v-list-item
v-for="mode_item in subset_edit_modes"
@mouseover="() => {hover_mode=mode_item.glue_name}"
@mouseleave="() => {if (hover_mode == mode_item.glue_name) {hover_mode=''}}"
>
<v-list-item-icon style="margin-right: 4px">
<img :src="mode_item.icon"/>
</v-list-item-icon>
<v-list-item-content>
<!--
<data-menu-subset-edit-modify
:mode_item="mode_item"
/>
-->
{{ mode_item.glue_name }}
</v-list-item-content>
<v-list-item-action style="display: inline-block" v-if="hover_mode == mode_item.glue_name">
<j-tooltip
v-for="tool in subset_tools.slice().reverse()"
:span_style="'display: inline-block; float: right;'"
:tooltipcontent="'Interactively apply \'' + mode_item.glue_name + '\' logic to ' + subset_selected + ' using the ' + tool.name + ' tool'"
>
<v-btn
icon
max-height="24px"
max-width="24px"
@click="() => {$emit('modify-subset', mode_item.glue_name, tool.name)}"
>
<img :src="tool.img" class="invert-if-dark" width="20"/>
</v-btn>
</j-tooltip>
</v-list-item-action>
</v-list-item>

</v-list>
</v-menu>
</template>

<script>
module.exports = {
data: function () {
return {
hover_mode: '',
}
},
props: ['subset_selected', 'subset_edit_enabled', 'subset_edit_tooltip', 'selected_n_subsets', 'subset_edit_modes', 'subset_tools'],
};
</script>

<style scoped>
.v-list-item__icon, .v-list-item__content, .v-list-item__action {
/* even denser than dense */
padding-top: 2px !important;
padding-bottom: 2px !important;
margin-top: 0px !important;
margin-bottom: 0px !important;
}
</style>
14 changes: 14 additions & 0 deletions jdaviz/components/plugin_live_results_icon.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<template>
<j-tooltip
tooltipcontent="plugin results automatically update"
span_style="display: inline-block"
>
<v-icon dense>mdi-reload-alert</v-icon>
</j-tooltip>
</template>

<script>
module.exports = {
props: []
};
</script>
Loading