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

Add tooltip plugin for AudioStream #77069

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented May 14, 2023

Follow-up to #63263
EDIT: Updated version here: #77069 (comment)

This PR adds a tooltip plugin for AudioStream resource. Normally the tooltip would show simply the length and visual preview of the stream, but that would be too boring, so I added playback functionality 🤪

ezgif.com-crop.mp4

It comes with play/pause, autoplay and 5s forward/backward seeking. I couldn't find a good way to add keyboard input to the tooltip, so I had to switch some flags and the tooltip causes some focus problems.

I initially came up with this idea as a fix for #63683, but #63263 wasn't merged in time and the issue was resolved in the meantime.

@KoBeWi KoBeWi added this to the 4.x milestone May 14, 2023
@KoBeWi KoBeWi requested a review from a team as a code owner May 14, 2023 15:23
@KoBeWi KoBeWi force-pushed the turning_tooltips_into_music_player_BECAUSE_WHY_NOT branch from 4270e80 to 8780f6f Compare May 14, 2023 15:26
@MewPurPur
Copy link
Contributor

MewPurPur commented May 14, 2023

Well, since there's no proposal, I'll leave my opinion here. I really appreciate the effort put into doing something like this with tooltips, but I don't think playback controls in them is a good idea.

  • The navigation is confusing, as to explain what each key does, you have to either use icons (visually intuitive, but can be confused for buttons, which is bad as tooltips aren't something you click) or text (can't be confused for buttons, but are more spacious - perhaps prohibitively so in some languages - and less visually intuitive).
  • It makes the tooltip bigger, covering more space.
  • This is the most important one: It is generally expected, for all tooltips right now, that they pop up temporarily when you hover something and never steal any focus. If my code editor is focused, even if I hover over an audio file in the FileSystem dock, the code editor should still consume all actions. I'd still expect that pressing "D" would write a "d" in the script, not play the audio.

Unrelated suggestion to my dissent above - having the length is nice, but I'd suggest that we:

  • Use minutes for tracks over 60sec.
  • Have two floating point digits for tracks under 1sec, else one floating point digit for tracks under 10sec, else none for tracks over 1min.

@KoBeWi
Copy link
Member Author

KoBeWi commented May 14, 2023

The navigation is confusing, as to explain what each key does, you have to either use icons (visually intuitive, but can be confused for buttons, which is bad as tooltips aren't something you click) or text (can't be confused for buttons, but are more spacious - perhaps prohibitively so in some languages - and less visually intuitive).

Well the letters next to icons imply enough that they are shortcuts, so even if someone tries to click it and fails, it's rather self-explanatory that you should use keyboard.

It makes the tooltip bigger, covering more space.

It's not a problem. The tooltip is still rather small overall.

This is the most important one: It is generally expected, for all tooltips right now, that they pop up temporarily when you hover something and never steal any focus. [...] I'd still expect that pressing "D" would write a "d" in the script, not play the audio.

IMO stealing key input is fine, because the tooltip is temporary. I can't imagine someone being bothered because they hovered an audio file while writing code and it made 4 letters unusable. Although focus stealing is a problem and I don't really know how to solve it. Perfectly the tooltip should only accept events, but right now it acts like a regular popup.

Use minutes for tracks over 60sec.
Have two floating point digits for tracks under 1sec, else one floating point digit for tracks under 10sec, else none for tracks over 1min.

Makes sense, although I'll do it a bit differently 🤔

tbh tooltip audio player is an overkill, especially with controls. It has one advantage though (at least if focus issues were resolved) - you can quickly preview multiple samples. I can't make it default to autoplay though, because it requires loading the audio stream.

This could be easily implemented in GDScript, but the way tooltip plugins are designed doesn't allow more than 1 to handle the same file type. Maybe the API could be changed a bit to allow chaining multiple plugins. They mostly use box containers anyway.

@MewPurPur
Copy link
Contributor

MewPurPur commented May 15, 2023

I think the dialog that opens on double-click, while not the prettiest, is quick enough to play several tracks for all practical purposes. So I'd still prefer to keep the convention of tooltips only showing info here.

Also if I get this right, there will be a possibility of a sound jumpscare after hovering an audio file (i.e. forgetting you enabled autoplay earlier)? I don't like the idea of this too.

Ik I probably sound a bit nitpicky, I just think the many little negatives of audio playback from tooltips outweigh the utility.

@KoBeWi
Copy link
Member Author

KoBeWi commented May 15, 2023

I think the dialog that opens on double-click, while not the prettiest, is quick enough to play several tracks for all practical purposes.

I'd agree if it had an autoplay or at least a shortcut of some sort. In the default layout it's even on the opposite side of the screen.

Also if I get this right, there will be a possibility of a sound jumpscare after hovering an audio file (i.e. forgetting you enabled autoplay earlier)?

The autoplay is only stored for the current editor session.

@KoBeWi KoBeWi force-pushed the turning_tooltips_into_music_player_BECAUSE_WHY_NOT branch from 8780f6f to a31470d Compare May 31, 2023 13:16
@KoBeWi
Copy link
Member Author

KoBeWi commented May 31, 2023

Removed the audio player, the tooltip now only includes length and stream image:
image
The branch name no longer makes sense :<

I noticed a problem though 🤔 The metadata is filled when the preview is generated, so if you have a cached preview, the tooltip will show length of 0. This also applies to texture tooltips. All previews generated before tooltip plugins were added will have invalid data. I'll try to invalidate it if the metadata is empty, but in another PR.

Here's a patch with rebased audio player btw:
tooltip music player lmao.patch
(I'm going to re-implement it in GDScript)

EDIT:
Length display as suggested by Mew:
image
image
image

@KoBeWi KoBeWi force-pushed the turning_tooltips_into_music_player_BECAUSE_WHY_NOT branch from a31470d to 0c30de3 Compare May 31, 2023 13:28
@KoBeWi KoBeWi force-pushed the turning_tooltips_into_music_player_BECAUSE_WHY_NOT branch from 0c30de3 to ae1d89c Compare April 6, 2024 15:44
@KoBeWi KoBeWi force-pushed the turning_tooltips_into_music_player_BECAUSE_WHY_NOT branch 3 times, most recently from 0e3bea0 to 9ed8429 Compare April 15, 2024 20:14
@KoBeWi KoBeWi force-pushed the turning_tooltips_into_music_player_BECAUSE_WHY_NOT branch from 9ed8429 to 1fce8d8 Compare April 15, 2024 20:14
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Apr 26, 2024
@akien-mga akien-mga merged commit 69a94c5 into godotengine:master Apr 26, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 26, 2024

btw there is complementary PR, because resource previews cached before this change will have outdated data: #77697
(I'll rebase it soon)

@KoBeWi KoBeWi deleted the turning_tooltips_into_music_player_BECAUSE_WHY_NOT branch April 26, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants