-
Notifications
You must be signed in to change notification settings - Fork 64
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
Changes from 19 commits
c8a156f
84eec12
0eba41a
785b55f
77984ae
7a69acf
9c68872
c0d6f8d
e67e7cc
226e748
f6bc113
051e055
c12b211
1abddc8
fdb751a
b8b9cc7
16bbeec
8bec53e
085f2df
b261a67
7b9366b
219b45a
bf59037
26e8efc
97cc23e
b0dccff
da18c17
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ? ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?: Maybe draft for me what you'd like to see or describe some more details. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
There was a problem hiding this comment.
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