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

Adds the ability to highlight regions using the bookmarks widget #4003

Merged
merged 27 commits into from
Dec 13, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
c8a156f
WIP: adding highlights to bookmarks
carolinebridge Oct 20, 2023
84eec12
Merge branch 'main' into bookmark-highlights
carolinebridge Nov 6, 2023
0eba41a
Adds label to highlights on the LGV; adds options to toggle on/off; a…
carolinebridge Nov 6, 2023
785b55f
Moving Highlight to GridBookmarkPlugin, lint, various checks and UI
carolinebridge Nov 9, 2023
77984ae
listen for storage events in other tabs
carolinebridge Nov 9, 2023
7a69acf
update menu items to submenu and tests
carolinebridge Nov 9, 2023
9c68872
make default color of highlight static
carolinebridge Nov 10, 2023
c0d6f8d
ensure highlights display on load new session
carolinebridge Nov 10, 2023
e67e7cc
edit dialog > highlight dialog
carolinebridge Nov 10, 2023
226e748
update tests
carolinebridge Nov 10, 2023
f6bc113
Avoid doing manual typing for callbacks
cmdcolin Nov 20, 2023
051e055
review feedback
carolinebridge Nov 22, 2023
c12b211
revert for lint
carolinebridge Nov 22, 2023
1abddc8
lint
carolinebridge Nov 22, 2023
fdb751a
type
carolinebridge Nov 22, 2023
b8b9cc7
rev changes to index.ts
carolinebridge Nov 22, 2023
16bbeec
Simplified behavior of the check for all views
cmdcolin Nov 23, 2023
8bec53e
possible side effect routine
carolinebridge Nov 23, 2023
085f2df
improving clear all and minor text edit
carolinebridge Nov 24, 2023
b261a67
lint and use colord
carolinebridge Nov 27, 2023
7b9366b
Merge branch 'main' into bookmark-highlights
carolinebridge Nov 27, 2023
219b45a
Use hamburger menu for bookmark options
cmdcolin Dec 13, 2023
bf59037
Attempt test fixes
cmdcolin Dec 13, 2023
26e8efc
Updates
cmdcolin Dec 13, 2023
97cc23e
Test bookmark widget using integration test
cmdcolin Dec 13, 2023
b0dccff
Misc
cmdcolin Dec 13, 2023
da18c17
Updates
cmdcolin Dec 13, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@ import {
measureGridWidth,
measureText,
} from '@jbrowse/core/util'
import { useResizeBar } from '@jbrowse/core/ui/useResizeBar'
import ResizeBar from '@jbrowse/core/ui/ResizeBar'

const ColorPicker = lazy(() => import('@jbrowse/core/ui/ColorPicker'))

// locals
import { navToBookmark } from '../utils'
import { GridBookmarkModel, IExtendedLabeledRegionModel } from '../model'
import { useResizeBar } from '@jbrowse/core/ui/useResizeBar'
import ResizeBar from '@jbrowse/core/ui/ResizeBar'

const EditBookmarkLabelDialog = lazy(() => import('./EditBookmarkLabelDialog'))

Expand Down Expand Up @@ -59,10 +61,8 @@ const BookmarkGrid = observer(function ({
}
})

// reset selections if bookmarked regions change
// needed especially if bookmarked regions are deleted, then
const [widths, setWidths] = useState([
50,
40,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that the checkbox cell is still width ~50 so this change may not be accurate. can see in hovering the lines are mismatched

image

Math.max(
measureText('Bookmark link', 12) + 30,
measureGridWidth(rows.map(row => row.locString)),
Expand All @@ -75,6 +75,7 @@ const BookmarkGrid = observer(function ({
measureText('Assembly', 12) + 30,
measureGridWidth(rows.map(row => row.assemblyName)),
),
100,
])

return (
Expand Down Expand Up @@ -123,6 +124,19 @@ const BookmarkGrid = observer(function ({
headerName: 'Assembly',
width: widths[3],
},
{
field: 'highlight',
headerName: 'Highlight',
width: widths[4],
renderCell: ({ value, row }) => (
<ColorPicker
color={value || 'black'}
onChange={event => {
return model.updateBookmarkHighlight(row, event)
carolinebridge marked this conversation as resolved.
Show resolved Hide resolved
}}
/>
),
},
]}
onCellDoubleClick={({ row }) => setDialogRow(row)}
processRowUpdate={row => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { makeStyles } from 'tss-react/mui'
import BookmarkGrid from './BookmarkGrid'
import DeleteBookmarks from './DeleteBookmarks'
import ExportBookmarks from './ExportBookmarks'
import HighlightBookmarks from './HighlightBookmarks'
import ImportBookmarks from './ImportBookmarks'
import AssemblySelector from './AssemblySelector'
import ShareBookmarks from './ShareBookmarks'
Expand Down Expand Up @@ -36,6 +37,7 @@ const GridBookmarkWidget = observer(function GridBookmarkWidget({
<ExportBookmarks model={model} />
<ImportBookmarks model={model} />
<ShareBookmarks model={model} />
<HighlightBookmarks model={model} />
<DeleteBookmarks model={model} />
</div>
<Alert severity="info">
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
import React, { useEffect, useRef } from 'react'
import { observer } from 'mobx-react'
import { makeStyles } from 'tss-react/mui'
import { SessionWithWidgets, getSession, notEmpty } from '@jbrowse/core/util'
import { IExtendedLGV } from '../../model'

// icons
import BookmarkIcon from '@mui/icons-material/Bookmark'

// locals
import { Tooltip } from '@mui/material'
import { GridBookmarkModel } from '../../model'

type LGV = IExtendedLGV

const useStyles = makeStyles()({
highlight: {
height: '100%',
position: 'absolute',
textAlign: 'center',
overflow: 'hidden',
display: 'flex',
alignItems: 'start',
},
})

const intensify = (rgba: string, opacity: number) => {
carolinebridge marked this conversation as resolved.
Show resolved Hide resolved
if (rgba) {
const values = rgba.replaceAll(/[^\d,.]/g, '').split(',')
const originalOpacity = values.pop()
// originalOpacity !== '0' assumes if the user has set the opacity of this highlight to 0, they probably don't want to see the label either
const n = `rgba(${values.join(', ')}, ${
originalOpacity !== '0' ? opacity : 0
})`
return n
}
return `rgba(0,0,0,0)`
}

const Highlight = observer(function Highlight({ model }: { model: LGV }) {
const { classes } = useStyles()

const session = getSession(model) as SessionWithWidgets
const { showBookmarkHighlights, showBookmarkLabels } = model
const assemblyNames = new Set(session.assemblyNames)

const bookmarkWidget = session.widgets.get(
'GridBookmark',
) as GridBookmarkModel

const bookmarks = useRef(bookmarkWidget?.bookmarks ?? [])

useEffect(() => {
if (!bookmarkWidget) {
const newBookmarkWidget = session.addWidget(
'GridBookmarkWidget',
'GridBookmark',
) as GridBookmarkModel
bookmarks.current = newBookmarkWidget.bookmarks
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think that this code points to a thing where the fact that the bookmarks are stored on the widget, and therefore needing the widget to be open, is a bit awkward.

one alternative approach would be to extend the session model (via Core-extendSession perhaps) and add the functions that read and manipulate the highlight state directly onto the session model. then, the highlights can be displayed regardless of whether the grid bookmark widget is opened.

i would say this change is not necessarily required for this PR to go forward but it could be worth considering for the future

Copy link
Collaborator

Choose a reason for hiding this comment

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

(note that one reason i say it's not strictly required is that the bookmark widget does not strictly need to even be "open" as in visible, as we have the concept of the widget state being retained in the session despite it being "closed", at least for now...xref #3538)

}
}, [session, bookmarkWidget])

return (
<>
{showBookmarkHighlights && bookmarks.current
? bookmarks.current
.filter(value => assemblyNames.has(value.assemblyName))
cmdcolin marked this conversation as resolved.
Show resolved Hide resolved
.map(r => {
const s = model.bpToPx({
refName: r.refName,
coord: r.start,
})
const e = model.bpToPx({
refName: r.refName,
coord: r.end,
})
return s && e
? {
width: Math.max(Math.abs(e.offsetPx - s.offsetPx), 3),
left: Math.min(s.offsetPx, e.offsetPx) - model.offsetPx,
highlight: r.highlight,
label: r.label,
}
: undefined
})
.filter(notEmpty)
.map(({ left, width, highlight, label }, idx) => (
<div
key={`${left}_${width}_${idx}`}
className={classes.highlight}
style={{ left, width, background: highlight }}
>
{showBookmarkLabels ? (
Copy link
Collaborator

Choose a reason for hiding this comment

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

you could consider drawing the text right onto the display. this would make it easier to see at a glance, and allow screenshot-ability compared to requiring a mouseover

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean have the label displayed at all times? is that desirable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO, yes. the bookmark icon feels odd to me, especially if there is not any label. i think it may look better for there to be no icon, and text displayed always, unless showBookmarkLabels is false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I agree with this --

You can turn the bookmark icon 'off' for the express reason of "I don't want this in my screenshots". Being enabled by default communicates to the user "what" the highlighted area is, and enables the label quick access. The icon, for this purpose, was requested by Lincoln when I first demo'd it to him (it looked like the first screenshot on this PR).

If a user really wants their labels in their screenshots, delayed screen captures exist. I think having so much text like that, especially for a heavy "annotator", would be obtuse.

Mostly though, I really don't see having the text displayed always as a better-looking option...I am having trouble conceiving what you might be imagining, here's a mockup?:
image
image

Maybe draft for me what you'd like to see or describe some more details.

Copy link
Collaborator

Choose a reason for hiding this comment

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

there are similar challenges with overlapping feature labels. I believe such overlapping text could be remedied in most cases by adding ellipses and/or perhaps adding y-axis dodging (so that they avoid being on the same text row)

i actually prefer the text being there despite overlaps, because it gives me a quick overview, without having to mouseover many things. it is up to you though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave it as is for now...I think resolving the overlaps with an ellipses negates the idea of having an overview of the labels (because you can't read them in their entirety), and if you're looking for a big picture overview, you can see all your labels in the widget. Y-axis dodging could resolve for some cases, but I don't think it would scale well, because in higher density you're adding a lot of vertical space clutter.

<Tooltip title={label} arrow>
<BookmarkIcon
fontSize="small"
sx={{ color: `${intensify(highlight, 0.8)}` }}
/>
</Tooltip>
) : null}
</div>
))
: null}
</>
)
})

export default Highlight
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import React from 'react'
import PluginManager from '@jbrowse/core/PluginManager'

// locals
import Highlight from './Highlight'
import { IExtendedLGV } from '../../model'

export default function AddHighlightModelF(pluginManager: PluginManager) {
pluginManager.addToExtensionPoint(
'LinearGenomeView-TracksContainerComponent',
// @ts-expect-error
(rest: React.ReactNode[] = [], { model }: { model: IExtendedLGV }) => {
return [
...rest,
<Highlight key={`highlight_grid_bookmark`} model={model} />,
]
},
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import React, { Suspense, lazy, useState } from 'react'
import { Button } from '@mui/material'

// icons
import EditIcon from '@mui/icons-material/Edit'

// locals
import { GridBookmarkModel } from '../model'
const HighlightBookmarksDialog = lazy(
() => import('./HighlightBookmarksDialog'),
)

function HighlightBookmarks({ model }: { model: GridBookmarkModel }) {
const [open, setOpen] = useState(false)
return (
<>
<Button
startIcon={<EditIcon />}
aria-label="edit bookmarks"
onClick={() => setOpen(true)}
>
Highlight
</Button>
{open ? (
<Suspense fallback={<React.Fragment />}>
<HighlightBookmarksDialog
model={model}
onClose={() => setOpen(false)}
/>
</Suspense>
) : null}
</>
)
}

export default HighlightBookmarks
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
import React, { useState } from 'react'
import { observer } from 'mobx-react'
import {
Button,
DialogContent,
DialogActions,
Alert,
Stack,
Typography,
Switch,
} from '@mui/material'
import { Dialog } from '@jbrowse/core/ui'
import { ColorPicker } from '@jbrowse/core/ui/ColorPicker'

// locals
import { GridBookmarkModel } from '../model'

const HighlightBookmarksDialog = observer(function ({
onClose,
model,
}: {
onClose: () => void
model: GridBookmarkModel
}) {
const { selectedBookmarks } = model
const editNone = selectedBookmarks.length === 0
const [color, setColor] = useState(
selectedBookmarks[0]?.highlight ?? 'rgba(247, 129, 192, 0.35)',
)

return (
<Dialog open onClose={onClose} title="Highlight bookmarks">
<DialogContent>
<Typography variant="h6">Bulk highlight selector</Typography>
<Alert severity="info">
{editNone ? (
<>
<span>
Use the checkboxes to select individual bookmarks to edit.
</span>
</>
) : (
'Only selected bookmarks will be edited.'
)}
</Alert>
{!editNone ? (
<ColorPicker
color={color}
onChange={event => {
setColor(event)
}}
/>
) : null}
<Typography variant="h6">Highlight toggles</Typography>
<Stack direction="row" alignItems="center">
<Switch
data-testid="toggle_highlight_all_switch"
checked={model.areBookmarksHighlightedOnAllOpenViews}
onChange={() => {
model.setHighlightToggle(
!model.areBookmarksHighlightedOnAllOpenViews,
)
}}
/>
<Typography variant="overline">
Toggle bookmark highlights on all open views
</Typography>
</Stack>
<Stack direction="row" alignItems="center">
<Switch
data-testid="toggle_highlight_label_all_switch"
checked={model.areBookmarksHighlightLabelsOnAllOpenViews}
onChange={() => {
model.setLabelToggle(
!model.areBookmarksHighlightLabelsOnAllOpenViews,
)
}}
/>
<Typography variant="overline">
Toggle 'bookmark' icon on LGV tracks
</Typography>
</Stack>
</DialogContent>
<DialogActions>
<Button variant="contained" color="secondary" onClick={() => onClose()}>
Cancel
</Button>
<Button
variant="contained"
color="primary"
onClick={() => {
model.updateBulkBookmarkHighlights(color)
onClose()
}}
>
Confirm
</Button>
</DialogActions>
</Dialog>
)
})

export default HighlightBookmarksDialog
6 changes: 5 additions & 1 deletion plugins/grid-bookmark/src/GridBookmarkWidget/index.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import { lazy } from 'react'
import { ConfigurationSchema } from '@jbrowse/core/configuration'
import { WidgetType } from '@jbrowse/core/pluggableElementTypes'
import stateModelFactory from './model'
import PluginManager from '@jbrowse/core/PluginManager'

// locals
import stateModelFactory from './model'
import AddHighlightModelF from './components/Highlight'

const configSchema = ConfigurationSchema('GridBookmarkWidget', {})

export default (pluginManager: PluginManager) => {
Expand All @@ -16,4 +19,5 @@ export default (pluginManager: PluginManager) => {
ReactComponent: lazy(() => import('./components/GridBookmarkWidget')),
})
})
AddHighlightModelF(pluginManager)
}
Loading
Loading