jbrowse-components icon indicating copy to clipboard operation
jbrowse-components copied to clipboard

Adds option to auto-adjust the track height to show all features

Open carolinebridge opened this issue 1 year ago • 20 comments
trafficstars

Adds new menu option to auto-adjust the track height to expand and shrink based on the features shown.

The functionality is as follows:

‘static’ === the track height will assume the height the user has adjusted the height to, or the configured height

‘dynamic’ === the track height will expand and shrink based on the height of the features shown

Resolves https://github.com/GMOD/jbrowse-components/issues/534

Adds a slot for this in the config:

    /**
     * #slot configuration.LinearGenomeViewPlugin.adjustTrackLayoutHeight
     */
    adjustTrackLayoutHeight: {
      type: 'string',
      defaultValue: 'static',
      model: types.enumeration('adjustTrackLayoutHeightOptions', [
        'static',
        'dynamic'
      ]),
    },

carolinebridge avatar Mar 01 '24 16:03 carolinebridge

@cmdcolin couple of questions:

  1. Where do you think it makes most sense to keep the setting for this? In the commit above it's in the display menu options (because I thought it made sense next to 'set max height'?), but an argument could be made for it in the view settings (because it's more 'universal') and we also discussed in the JBrowse settings. Some visuals:

Option 1: JBrowse settings image

Option 2: View settings image

Options 3: Display settings image

  1. Can you define what you mean/want out of 'on first render'?

  2. Can you comment on the implementation of the setting -- I mirrored the setting for track labels, but then noticed you had some scaffolding on this branch for using a disposer on the web session model...track labels operates similarly (semi-universal setting on the tracks), but its placement is kind of odd like that (a track-level setting, in the view menu, that applies to the whole app until changed)

carolinebridge avatar Mar 01 '24 16:03 carolinebridge

Codecov Report

Attention: Patch coverage is 59.64912% with 23 lines in your changes missing coverage. Please review.

Project coverage is 62.62%. Comparing base (a097560) to head (49214ed). Report is 115 commits behind head on main.

:exclamation: Current head 49214ed differs from pull request most recent head 6ff2f10

Please upload reports for the commit 6ff2f10 to get more accurate results.

Files Patch % Lines
.../src/BaseLinearDisplay/models/TrackHeightMixin.tsx 44.44% 15 Missing :warning:
...s/linear-genome-view/src/LinearGenomeView/model.ts 40.00% 6 Missing :warning:
...ments/src/LinearAlignmentsDisplay/models/model.tsx 81.81% 2 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4252      +/-   ##
==========================================
+ Coverage   62.61%   62.62%   +0.01%     
==========================================
  Files        1088     1086       -2     
  Lines       31495    31442      -53     
  Branches     7531     7507      -24     
==========================================
- Hits        19721    19692      -29     
+ Misses      11602    11578      -24     
  Partials      172      172              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 01 '24 16:03 codecov[bot]

  1. I dont quite know the best but the "View menu" might be a happy medium between per-track and global. Can make it a localstorage setting so once you set it on one view, it is "sticky" to other future instances of other views and sessions.
  2. given that it is a block based thing, perhaps it would mean "max height after all blocks in view are rendered". hard to maybe do that codewise so it could be "first block" but ideal i think is taking max layout height of all visible blocks after the first render of all blocks.
  3. the session model code that was there was an errant commit that was for unrelated thing, i didn't mean to include it in my scaffolding (can remove it)

cmdcolin avatar Mar 04 '24 16:03 cmdcolin

can you post a couple session links that demonstrate the usage of this feature?

cmdcolin avatar Mar 07 '24 21:03 cmdcolin

@cmdcolin

  1. Session with two views and tracks open, auto-adjust to show all features on the track is enabled on both views, and you can observe the track height is different for the two views, adjusted to show all features on the track: https://s3.amazonaws.com/jbrowse.org/code/jb2/layout_max_height_calc/index.html?session=share-Pd1tUMeUnd&password=Vp8DR
  2. Session with two views and tracks open, auto-adjust to show height of initial features is enabled on the top view, and auto-adjust to show all features on the track is enabled on the bottom view. Scroll to the left and you'll notice the track height does not adjust to show all features like the bottom view shows. https://s3.amazonaws.com/jbrowse.org/code/jb2/layout_max_height_calc/index.html?session=share-mxG8gyfp70&password=oxCfG

carolinebridge avatar Mar 08 '24 12:03 carolinebridge

This is great to see, it will be a nice option to have. Some things I noticed while trying out this branch:

  • This doesn't seem to work with alignments displays
  • "auto-adjust to show all features on the track" will expand the track to show more features, but not shrink the track when there are fewer features
    • Wasn't sure if this was intended behavior or not
    • If we make it shrink as well as expand, we can disable the track resizing drag handle when in this mode
  • Using the track resize drag handle to resize the track causes unexpected behavior
    • If you use the drag handle resize the track with "auto-adjust to show all features on the track" on, the auto-adjusting stops working
    • If you use the drag handle to resize the track in any mode, switching between track height modes no longer does anything, you have to turn the track off and on again to reset it
  • Even with auto resizing, there is always a scroll bar on the SVG tracks unless you manually expand the track. I think this is due to the padding that SVG tracks use to ensure there is somewhere to click so you can de-select a feature: https://github.com/GMOD/jbrowse-components/blob/8355237c871d4d87a082c2f24f14ac01cefe70ff/plugins/svg/src/SvgFeatureRenderer/components/SvgFeatureRendering.tsx#L24-L26 It might be good to take this padding into account when sizing the track.

garrettjstevens avatar Mar 08 '24 19:03 garrettjstevens

@garrettjstevens

This doesn't seem to work with alignments displays

Fixed, should work with alignments displays now

"auto-adjust to show all features on the track" will expand the track to show more features, but not shrink the track when there are fewer features Wasn't sure if this was intended behavior or not If we make it shrink as well as expand, we can disable the track resizing drag handle when in this mode

Going to leave as-is for now, because the layout height "only increases"

Using the track resize drag handle to resize the track causes unexpected behavior If you use the drag handle resize the track with "auto-adjust to show all features on the track" on, the auto-adjusting stops working If you use the drag handle to resize the track in any mode, switching between track height modes no longer does anything, you have to turn the track off and on again to reset it

Now, if the user resizes their track while one of the auto-adjust settings are activated, the setting will change to "off" with a notification to the user. The user can then change the setting back if they wish. I think this is a good solution because it will always do what the user is asking (that is, maintain the track height they have set it to).

Let me know what you think.

Even with auto resizing, there is always a scroll bar on the SVG tracks unless you manually expand the track. I think this is due to the padding that SVG tracks use to ensure there is somewhere to click so you can de-select a feature [...]

The +100 magic number has been added to the layout height calc so the scroll bar will no longer appear when the user is making use of these settings.

carolinebridge avatar Mar 15 '24 15:03 carolinebridge

Some comments from Lincoln:

  1. Add an option such that the track height is expandable but bounded by a value specified by the user
  2. Shrinking and expanding with the user's navigation is a must
  3. Add a hotkey (like a double click, or a shift click) that expands to max height or retracts a track (like double clicking on a browser window expands it)
  4. The "first render" option might see limited use

carolinebridge avatar Mar 20 '24 18:03 carolinebridge

Demo: https://s3.amazonaws.com/jbrowse.org/code/jb2/layout_max_height_calc/index.html?session=share-cTpZQPAt3Z&password=8epM2

Clear your lgv-adjustTrackLayoutHeight localStorage setting so the old terms don't mess it up.

The following changes have been made:

  • Settings have been changed to be 'static', 'dynamic', and 'bound' (see the PR desc. for definitions)
  • 'dynamic' now shrinks and expands with the height of the currently viewable blocks

Let me know how it feels; from a usability standpoint, does this make sense as a universal setting, like the track labels setting?

Something I am uncertain about is setting to 'dynamic', then resizing a single track will apply 'static' to all the tracks...is that what the user wants? I like the visibility in the view menu, but is this perhaps something a user would want to apply per-track?

Also wondering about the "bound" setting being based on the track height configuration...is that accessible to a user who would be interested in that setting?

carolinebridge avatar Mar 29 '24 15:03 carolinebridge

nice work...random test, I tried changing to "compact" features here but the height did not adjust to the new shorter height of the track after applying compact

https://jbrowse.org/code/jb2/layout_max_height_calc/?config=%2Fgenomes%2FGRCh38%2F1000genomes%2Fconfig_1000genomes.json&session=share-aZWHI-8%2B%2Bu&password=uqy0x

i originally visited https://jbrowse.org/code/jb2/v2.10.3/?config=%2Fgenomes%2FGRCh38%2F1000genomes%2Fconfig_1000genomes.json&session=share-SUK-mntGyB&password=eQF0F

and then swapped this branch name in the url https://jbrowse.org/code/jb2/layout_max_height_calc/?config=%2Fgenomes%2FGRCh38%2F1000genomes%2Fconfig_1000genomes.json&session=share-SUK-mntGyB&password=eQF0F

also changing to "dynamic" increases the size of the snpcoverage subtrack

cmdcolin avatar Apr 11 '24 18:04 cmdcolin

Possibly off-topic, but does this PR allow this track height setting to be configured in a config.json?

nathanweeks avatar Apr 17 '24 21:04 nathanweeks

@nathanweeks configuring the track height in config.json is already allowed in released versions, and it should remain allowed even after this PR :)

an example of config.json with height preconfigured looks like this, it is a "display" config

{
      "type": "AlignmentsTrack",
      "trackId": "mytrack",
      "name": "my track",
      "assemblyNames": ["volvox"],
      "adapter": {
        "type": "CramAdapter",
        "cramLocation": {"uri": "volvox-sorted-altname.cram"},
        "craiLocation": {"uri": "volvox-sorted-altname.cram.crai"},
        "sequenceAdapter": {
          "type": "TwoBitAdapter",
          "twoBitLocation": {
            "uri": "volvox.2bit",
            "locationType": "UriLocation"
          }
        }
      },
      "displays": [
        {
          "type": "LinearPileupDisplay",
          "displayId": "mytrack_pileup",
          "height": 400
        }
      ]
    },

cmdcolin avatar Apr 17 '24 23:04 cmdcolin

Thanks @cmdcolin! I now realize the distinction between track "height" and "max height" --- the latter was what I meant.

Adapting your track height example, the following seemd to work for "max height":

    {
      "type": "FeatureTrack",
      "trackId": "mytrack",
      "name": "my track",
      "adapter": {
        "type": "Gff3TabixAdapter",
        "gffGzLocation": {
          "uri": "https://example.org/my.gff3.gz",
          "locationType": "UriLocation"
        },
        "index": {
          "location": {
            "uri": "https://example.org/my.gff3.gz.tbi",
            "locationType": "UriLocation"
          },
          "indexType": "TBI"
        }
      },
      "category": ["Genes"],
      "assemblyNames": ["myassembly"],
      "displays": [
        {
          "displayId": "mytrack_displayId",
          "renderer": {
            "maxHeight": 3000
          }
        }
      ]
    },

A more-relevant follow-on: will the default value for the "auto-adjust track height" feature proposed in this PR be configurable for a given track in config.json, so users wouldn't necessarily have to discover it in the menu?

nathanweeks avatar Apr 19 '24 15:04 nathanweeks

Thanks @cmdcolin! I now realize the distinction between track "height" and "max height" --- the latter was what I meant.

Adapting your track height example, the following seemed to work for "max height":

    {
      "type": "FeatureTrack",
      "trackId": "mytrack",
      "name": "my track",
      "adapter": {
        "type": "Gff3TabixAdapter",
        "gffGzLocation": {
          "uri": "https://example.org/my.gff3.gz",
          "locationType": "UriLocation"
        },
        "index": {
          "location": {
            "uri": "https://example.org/my.gff3.gz.tbi",
            "locationType": "UriLocation"
          },
          "indexType": "TBI"
        }
      },
      "category": ["Genes"],
      "assemblyNames": ["myassembly"],
      "displays": [
        {
          "displayId": "mytrack_displayId",
          "renderer": {
            "maxHeight": 3000
          }
        }
      ]
    },

A more-relevant follow-on question: will the default value for the "auto-adjust track height" feature proposed in this PR be configurable for a given track in config.json, so users wouldn't necessarily have to discover it in the menu?

nathanweeks avatar Apr 19 '24 15:04 nathanweeks

will the default value for the "auto-adjust track height" feature proposed in this PR be configurable for a given track in config.json

currently that is not part of this PR. I think this is worth discussing though.

cmdcolin avatar Apr 22 '24 16:04 cmdcolin

example of where per-track setting might be relevant

a) situation like @nathanweeks refers to where admin might want it pre-configured b) situation where currently in this PR, clicking and dragging a particular tracks height changes the "dynamic" to "static" for all tracks. this seems somewhat unexpected in a way

possible way to help address this in the UI: the vertical resize handle on each track has a small "indicator" (not sure how it would look) that shows whether it is dynamic or static, and can be toggled on and off with a click.

cmdcolin avatar Apr 22 '24 16:04 cmdcolin

i think also the height calculation on the blocks might still have a race condition. from quickly moving around example screenshot below

Screenshot from 2024-04-19 21-39-52

cmdcolin avatar Apr 22 '24 16:04 cmdcolin

will the default value for the "auto-adjust track height" feature proposed in this PR be configurable for a given track in config.json

It is configurable on a per-session basis as part of this PR -- like track labels orientation -- I would say we should consider either making it configurable per-session, or configurable per-track. Thoughts, @cmdcolin ?

carolinebridge avatar Apr 22 '24 16:04 carolinebridge

it could be worth experimenting with per track to see how it feels

cmdcolin avatar Apr 22 '24 17:04 cmdcolin

Demo of per-track settings https://s3.amazonaws.com/jbrowse.org/code/jb2/layout_max_height_calc/index.html?session=share-fFEMRjtavP&password=NXDQb

carolinebridge avatar May 06 '24 14:05 carolinebridge

close for now...will probably want to be revisited eventually

cmdcolin avatar Nov 15 '25 16:11 cmdcolin