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 option to auto-adjust the track height to show all features #4252

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

carolinebridge
Copy link
Contributor

@carolinebridge carolinebridge commented Mar 1, 2024

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 #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 carolinebridge added the enhancement New feature or request label Mar 1, 2024
@carolinebridge carolinebridge self-assigned this Mar 1, 2024
@carolinebridge
Copy link
Contributor Author

carolinebridge commented Mar 1, 2024

@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)

Copy link

codecov bot commented Mar 1, 2024

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.

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 ⚠️
...s/linear-genome-view/src/LinearGenomeView/model.ts 40.00% 6 Missing ⚠️
...ments/src/LinearAlignmentsDisplay/models/model.tsx 81.81% 2 Missing ⚠️
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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cmdcolin
Copy link
Collaborator

cmdcolin commented Mar 4, 2024

  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)

@carolinebridge carolinebridge changed the title WIP: Adds option to auto-adjust the track height to show all features Adds option to auto-adjust the track height to show all features Mar 5, 2024
…rst render" i.e. all blocks are defined, then determine track height ;; renames and redefines some getters
@carolinebridge carolinebridge marked this pull request as ready for review March 5, 2024 21:45
@cmdcolin
Copy link
Collaborator

cmdcolin commented Mar 7, 2024

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

@carolinebridge
Copy link
Contributor Author

carolinebridge commented Mar 8, 2024

@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

@garrettjstevens
Copy link
Collaborator

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:
    // used so that user can click-away-from-feature below the laid out features
    // (issue #1248)
    const svgHeightPadding = 100

    It might be good to take this padding into account when sizing the track.

@carolinebridge
Copy link
Contributor Author

carolinebridge commented Mar 15, 2024

@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
Copy link
Contributor Author

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 carolinebridge marked this pull request as draft March 21, 2024 19:28
@cmdcolin
Copy link
Collaborator

@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
        }
      ]
    },

@nathanweeks
Copy link
Member

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
Copy link
Member

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?

@cmdcolin
Copy link
Collaborator

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
Copy link
Collaborator

cmdcolin commented Apr 22, 2024

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
Copy link
Collaborator

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

@carolinebridge
Copy link
Contributor Author

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 ?

@cmdcolin
Copy link
Collaborator

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

@carolinebridge
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-adjust track heights to their current layout height
4 participants