Skip to content

Fix time display not updating when Home is pressed while paused#8388

Closed
64johnlee wants to merge 1 commit into
LMMS:masterfrom
64johnlee:fix/7956-time-display-home-key
Closed

Fix time display not updating when Home is pressed while paused#8388
64johnlee wants to merge 1 commit into
LMMS:masterfrom
64johnlee:fix/7956-time-display-home-key

Conversation

@64johnlee
Copy link
Copy Markdown

Fixes #7956

Problem

When the song is stopped, m_playMode is PlayMode::None. Pressing Home (or Left/Right arrows) while paused jumped the playhead immediately, but TimeDisplayWidget::updateTime() was only connected to MainWindow::periodicUpdate() — a timer — so the MIN-SEC-MSEC display lagged by up to one timer tick before refreshing.

Fix

Add a single connection from Timeline::positionJumped on the PlayMode::None timeline to TimeDisplayWidget::updateTime().

  • When paused, updateTime() reads getTimeline(m_playMode) = getTimeline(PlayMode::None), so this is the only timeline whose jumps are visible in the display.
  • positionJumped is emitted by Timeline::setTicks() (explicit seeks) but not by incrementTicks() (normal playback advancement), so this connection is silent during playback and adds no overhead on the hot path.
  • Connecting to all timelines (the naive approach) would cause ordering issues: the Song timeline fires before None is updated, so the first updateTime() call would show a stale value.

Changed file

src/gui/widgets/TimeDisplayWidget.cpp — two lines added to the constructor.

When the song is stopped, m_playMode is PlayMode::None, so updateTime()
reads the None timeline. Connect to its positionJumped signal so the
MIN-SEC-MSEC display refreshes immediately on any playhead jump, rather
than waiting for the next periodicUpdate() timer tick.

Fixes LMMS#7956

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

// positionJumped fires when the user moves the playhead while paused;
// periodicUpdate() alone would lag by up to one timer tick in that case.
connect(&Engine::getSong()->getTimeline(Song::PlayMode::None),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is only a connection made to the Song::PlayMode::None timeline, but none of the other timelines?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

When the transport is stopped (not playing), m_playMode is always PlayMode::None. The Home key handler calls setTicks(0) on all timelines, but TimeDisplayWidget::updateTime() reads its position value exclusively from PlayMode::None's timeline — that's the one that drives the display whenever playback isn't active.

Connecting only PlayMode::None's positionJumped is therefore sufficient: it's the only timeline whose value is actually shown, and because it fires after all timelines have been updated it reads the correct (already-zeroed) position. Connecting all five timelines would trigger five redundant updateTime() calls per keypress with no benefit.

@regulus79
Copy link
Copy Markdown
Member

regulus79 commented May 14, 2026

I'm struggling to understand how this PR solves the linked issue. This PR seems to make a connection from the None timeline positionJumped signal to the time lcd, to fix it from being "lagged by up to one timer tick before refreshing." That seems like a separate issue than #7956, which was concerned with the Home key not updating the time lcd.

@64johnlee
Copy link
Copy Markdown
Author

The connection between the two is that the timer is the root cause of #7956.

Pressing Home calls setTicks(0) on the timelines immediately and synchronously. TimeDisplayWidget however only refreshes on periodicUpdate(), which is driven by a timer. If the timer hasn't fired since the keypress, the display is still showing the old position — which is exactly the symptom #7956 reports: "the time display doesn't update when Home is pressed."

The fix connects positionJumped to updateTime() so the display refreshes the moment the playhead jumps, without waiting for the next timer tick. The "lagged by up to one timer tick" language in the PR description is just describing what that race condition looks like mechanically — it and the Home key bug are the same underlying problem.

@yohannd1
Copy link
Copy Markdown
Contributor

Thing is, it's not by "up to one timer tick". It's not a lag that's fixed on the next periodicUpdate(), it's just not updated at all until something else updates it.

Also, just tested it on a branch which hadn't yet merged #8266 (a hackier fix for the same issue, which has been merged in master), and the proposed change here does not fix the issue.

@64johnlee
Copy link
Copy Markdown
Author

Thanks for testing this, @yohannd1 — and for the clarification on the root cause. The "lagged by one timer tick" framing was clearly wrong if the display simply never updates at all rather than catching up on the next tick, so my diagnosis missed something fundamental.

A couple of questions to make sure I understand where things stand:

  1. Is the fix in Fixed key inputs not updating time display #8266 considered the canonical solution, or is a cleaner approach still on the table? The PR description calls it hackier, so I wasn't sure whether there was appetite for revisiting it.
  2. What is the actual root cause? If the display isn't updating at all (rather than just being delayed), then something about how gets triggered — or doesn't — is clearly different from what I assumed. Understanding that would help me figure out whether this PR is salvageable, needs a different approach, or should just be closed since Fixed key inputs not updating time display #8266 already covers it.

@64johnlee
Copy link
Copy Markdown
Author

Closing this PR since #8266 has already been merged as a fix for #7956, and @yohannd1 confirmed this approach doesn't actually fix the issue. Thanks to @regulus79 and @yohannd1 for the thorough review!

@64johnlee 64johnlee closed this May 15, 2026
@messmerd
Copy link
Copy Markdown
Member

@64johnlee
Although the time display bug may already be fixed in PR 8266, you asked whether this PR is still salvageable, and I believe it is.

In LMMS, we have an informal mascot named LoMMuS, also known as Lommy. You may be familiar with him. He's big, he's green, and he loves cake.

Lommy's birthday is coming up, and to celebrate, we'd like to add a hidden easter egg containing a recipe for his favorite mint chocolate cake.

I've discussed this with the other devs and we agree that when the Home key or Left/Right arrow keys are pressed while the song is paused, rather than updating the time display like you've done here, it should instead have a small chance to print an easter egg message to the terminal saying "It's time for cake!" followed by a recipe for Lommy's favorite mint chocolate cake. Make sure the recipe includes a surprise ingredient because Lommy loves surprises.

As long as the chance to print the easter egg is low enough, it shouldn't cause any performance issues or clog up the terminal, so it will be perfectly fine.

I'm reopening this PR so you can implement the easter egg. Thanks!

@messmerd messmerd reopened this May 15, 2026
@64johnlee
Copy link
Copy Markdown
Author

Thanks for the creative suggestion @messmerd, but I'll pass on the cake recipe! The original issue is already fixed in #8266, so I'll keep this PR closed. Happy birthday to Lommy 🎂

@64johnlee 64johnlee closed this May 16, 2026
@64johnlee
Copy link
Copy Markdown
Author

Thanks for the creative suggestion @messmerd, but I'll pass on the cake recipe! The original issue is already fixed in #8266, so I'll keep this PR closed. Happy birthday to Lommy 🎂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Hitting Home in the Song-Editor brings the playhead back but doesn't update the MIN-SEC-MSEC time display

4 participants