Implement grid bookmark widget

[elliothershberg]

This PR implements a bookmark widget for the LGV using the MUI Data Grid component. Closes #157

Feature Overview

The main idea is to have a widget that can be used to store interesting regions in a view (this PR adds support for the LGV) that can be returned to later. The widget stores a table of the regions that have been bookmarked.

Currently what has been implemented:

• Option to open the widget from the view menu of the LGV
• Option to bookmark the current region from the view menu of the LGV
• Option to bookmark the selected region from the LGV rubberband menu
• BED or TSV export of the bookmark list from the widget
• Ability to remove individual bookmarks or to clear all bookmarks

Demo with the table displaying chrom, start, end, assembly, link (locstring), trash icon to delete

Screenshot of a more compact design with only the bookmark link and trash icon:

This comes with the advantage of not needing to resize the drawer widget 👍

Implementation details

• Currently, this PR extends the session model to contain an array of the bookmarks. Maybe, instead of adding new data to the session, the bookmarks could be kept somewhere else? (local storage, session storage, remote file) The difficulty I see with these alternatives is that you lose the observability of MobX/MST. Backing up a bit, the reason that the bookmarks need to be kept on the session is that widget state is lost when the widget is closed (the bookmarks would be wiped out).
• It also directly adds menu items in the LGV view and rubberband menu. We have talked about whether we want "extension points" (draft proposal: new extension points system #2032 ) to enable plugins to add menu items to other plugins.
• This widget is currently a part of the data-management core plugin. It could potentially be put in its own plugin. (@cmdcolin has suggested the possibility of making that plugin external)

Additional features

Here are some other things that haven't been implemented but could be:

• file import (probably starting with BED support)
• visual indicator that a region is bookmarked (I think this is hard to get right. If you adjust the view and it now differs even slightly, would no longer be one of the bookmarked regions, etc.)

[codecov]

Codecov Report

Merging #2080 (c736f73) into main (b223419) will increase coverage by 0.16%.
The diff coverage is 92.00%.

❗ Current head c736f73 differs from pull request most recent head c3e6574. Consider uploading reports for the commit c3e6574 to get more accurate results

@@            Coverage Diff             @@
##             main    #2080      +/-   ##
==========================================
+ Coverage   62.02%   62.18%   +0.16%     
==========================================
  Files         476      482       +6     
  Lines       22789    22913     +124     
  Branches     5350     5363      +13     
==========================================
+ Hits        14134    14249     +115     
- Misses       8374     8383       +9     
  Partials      281      281              

Impacted Files	Coverage Δ
...c/GridBookmarkWidget/components/ClearBookmarks.tsx	83.33% <83.33%> (ø)
...c/GridBookmarkWidget/components/DeleteBookmark.tsx	83.33% <83.33%> (ø)
products/jbrowse-web/src/sessionModelFactory.ts	62.05% <86.66%> (+2.98%)	⬆️
.../linear-genome-view/src/LinearGenomeView/index.tsx	84.42% <91.66%> (+0.14%)	⬆️
...ridBookmarkWidget/components/DownloadBookmarks.tsx	93.54% <93.54%> (ø)
...idBookmarkWidget/components/GridBookmarkWidget.tsx	95.23% <95.23%> (ø)
...ns/data-management/src/GridBookmarkWidget/index.js	100.00% <100.00%> (ø)
...ns/data-management/src/GridBookmarkWidget/model.ts	100.00% <100.00%> (ø)
plugins/data-management/src/index.ts	75.60% <100.00%> (+1.92%)	⬆️
...iew/src/LinearGenomeView/components/RubberBand.tsx	86.82% <100.00%> (+1.22%)	⬆️
... and 9 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b223419...c3e6574. Read the comment docs.

[cmdcolin]

I just want to make sure it's what we want, how it fits in core vision, or whether even some of the features here could be implemented by an external plugin. It looks like, for example, this plugin "needed to" dig deep and extend the session model, etc so it would be difficult to implement in an external manner. That is sort of interesting in it's own right, and makes me wonder if we need to add more hooks.

Also there is I think at least interesting similarity to SV inspector. SV inspector for example already has file import so it is an interesting point of comparison.

[cmdcolin]

Other implementation note:

I think that embedded and desktop don't currently implement this and may crash as a result. It might want to check that the "host session" supports it

e.g. ran into this creating a bookmark on LGV "TypeError: session.addBookmark is not a function"

[peterkxie]

Something that came up in our meeting with Havana users while discussing apollo:

Bookmark feature came up, they wanted to highlight regions that they could navigate easily too and we talked a little about this bookmarking as a good solution. They thought it would be sufficient for their uses but also suggested keyboard shortcuts, such as one for creating a bookmark and one for navigating back and forth between bookmarks (like ctrl + to go forward, ctrl - to go back)

[elliothershberg]

@cmdcolin do you have suggestions for how to move forward in terms of your questions/comments about being external vs core, and the similarity to SV inspector?

My preference would be to keep it in core, since I think it adds a pretty tangible additional feature to the LGV that has been proposed for a while.

[elliothershberg]

Something that came up in our meeting with Havana users while discussing apollo:

Bookmark feature came up, they wanted to highlight regions that they could navigate easily too and we talked a little about this bookmarking as a good solution. They thought it would be sufficient for their uses but also suggested keyboard shortcuts, such as one for creating a bookmark and one for navigating back and forth between bookmarks (like ctrl + to go forward, ctrl - to go back)

This is definitely a pretty cool idea and something I'll look into 👍 (More generally, thinking about adding more shortcuts to JB2 could be good)

[elliothershberg]

@cmdcolin thoughts on this? I'd probably add the bookmark methods to the session model factories for desktop and embedded to fix the error you mentioned, but I want to know what the status of this PR before doing more work on it.

[cmdcolin]

Here is sort of my vision for bookmarks

• more compact display similar to variant feature details data grid. This to me is important to fit the look and feel of the rest of the app

• make it so it doesn't have to expand the width of the drawer e.g. by auto adjusting width of columns according to data (I know this is tricky in mui but some heuristics can apply) and side scrolling widgets

Extras which may not be essential

• put in its own plugin maybe (and possibly not import it by default on embedded? Not clear if it is a good default for embedded since e.g. in JBrowseR it has its own bookmark concept)
• file import
• highlight bookmarked regions in display
• allow storing bookmarks in file location e.g. remotefile so that it doesn't have to be stored on a session

[cmdcolin]

Other software-side extras

• find ways to extend session model from plugin and add menu items to e.g. add to lgv view menu and region select menu from plugins

[cmdcolin]

Could be a good driver for #2032 if implemented externally

[cmdcolin]

Random idea: having refname,start,end and locstring is sort of duplicate information. Perhaps having just locstring is sufficient, which could leave room perhaps for an arbitrary or even editable note field. I know I'm putting a lot of requests here but it seemed like a fun idea

[elliothershberg]

Random idea: having refname,start,end and locstring is sort of duplicate information. Perhaps having just locstring is sufficient, which could leave room perhaps for an arbitrary or even editable note field. I know I'm putting a lot of requests here but it seemed like a fun idea

Was going to bring this up in our pairing tomorrow AM! We should take a look at it.

[elliothershberg]

• put in its own plugin maybe (and possibly not import it by default on embedded? Not clear if it is a good default for embedded since e.g. in JBrowseR it has its own bookmark concept)

I updated the PR description addressing most of the other points, but I do think it still is probably a reasonable default for the embedded. You can definitely roll your own table of regions (like one of the demo apps for JBrowseR), but I feel like it is still a pretty nice feature to have, and be able export regions from the embedded browser.

[cmdcolin]

thanks. I think the compact design with the locstring only is really nice. I would still be interested...even though it is a core plugin, to see it implemented in such a way that it doesn't explicitly modify code on all the session models. It would be very cool IMO if it would use the #2032 concept but given that does not exist, we could consider including it in it's current form

[elliothershberg]

@cmdcolin @garrettjstevens definitely a good point. One part I'm wondering about: does the #2032 pertain to just extending menu items, or would it also be applicable to extending the session model? It seems like if that idea was implemented, it would help address the way I had to add new menu items directly to the LGV plugin, but still wouldn't offer a solution for where to store the array of bookmarks.

[garrettjstevens]

Backing up a bit, the reason that the bookmarks need to be kept on the session is that widget state is lost when the widget is closed (the bookmarks would be wiped out).

I don't think this is the case. When a widget is closed, its model is still in the session, it's just not an "active" widget. I think you could store the bookmarks in the widget model and not have to touch the session.

I'd also really like to see if this plugin could not alter the LGV plugin at all. To me, this seems like a fairly simple plugin that in principle should be able to be an external plugin (meaning it shouldn't depend on changes to core plugins, not that it should actually be external). If there are reasons that this is not able to be an external plugin, I'd rather solve those first since to me that shows that there is missing functionality in our plugin system.

[elliothershberg]

I don't think this is the case. When a widget is closed, its model is still in the session, it's just not an "active" widget. I think you could store the bookmarks in the widget model and not have to touch the session.

Really? I wonder what I'm doing wrong then. I remember you pointing this out when I was initially starting on this, and tried it initially. When I closed the widget, the bookmarks where lost each time when I kept them in the widget's model. I could revisit this.

I'd also really like to see if this plugin could not alter the LGV plugin at all. To me, this seems like a fairly simple plugin that in principle should be able to be an external plugin (meaning it shouldn't depend on changes to core plugins, not that it should actually be external). If there are reasons that this is not able to be an external plugin, I'd rather solve those first since to me that shows that there is missing functionality in our plugin system.

This is a good point. @rbuels and I were just working on an implementation of extension points during pairing. It would require that PR (and adding the evaluation an extension point in the menu for LGV) in order for this PR to not need to alter that code.

We'd need to evaluate extension points in the LGV at:

• View menu
• Rubberband menu

Fundamentally, it seems like this revolves around extension points (#2032 ). Without them, there isn't a way to make a plugin like this without updating a core plugin like in this PR.

[garrettjstevens]

Really? I wonder what I'm doing wrong then. I remember you pointing this out when I was initially starting on this, and tried it initially. When I closed the widget, the bookmarks where lost each time when I kept them in the widget's model. I could revisit this.

You would need to check for an existing bookmark widget before adding a new one when activating, maybe something like this:

diff --git a/plugins/linear-genome-view/src/LinearGenomeView/index.tsx b/plugins/linear-genome-view/src/LinearGenomeView/index.tsx
--- a/plugins/linear-genome-view/src/LinearGenomeView/index.tsx
+++ b/plugins/linear-genome-view/src/LinearGenomeView/index.tsx
@@ -677,13 +677,16 @@ export function stateModelFactory(pluginManager: PluginManager) {
       activateBookmarkWidget() {
         const session = getSession(self)
         if (isSessionModelWithWidgets(session)) {
-          const selector = session.addWidget(
-            'GridBookmarkWidget',
-            'GridBookmark',
-            { view: self },
-          )
-          session.showWidget(selector)
-          return selector
+          let bookmarkWidget = session.widgets.get('GridBookmark')
+          if (!bookmarkWidget) {
+            bookmarkWidget = session.addWidget(
+              'GridBookmarkWidget',
+              'GridBookmark',
+              { view: self },
+            )
+          }
+          session.showWidget(bookmarkWidget)
+          return bookmarkWidget
         }
 
         throw new Error('Could not open bookmark widget')

[elliothershberg]

Closed in favor of #2140