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

main toolbar rework (integrate player dialog) #544

Merged
merged 2 commits into from
Nov 6, 2024

Conversation

guiv42
Copy link
Collaborator

@guiv42 guiv42 commented Oct 6, 2024

see #533
@helge17: I would like to have your opinion before merging, it's always highly subjective when it comes to user interface.
This evolution targets a version 2.0 and is inspired by 2.0beta fork, but I'm following a more conservative path.

Timestamp display is the most challenging evolution:

  • font is configurable, just click on timestamp
  • most probably a specific setting will be needed for font on macOS, could you please have a look?
  • foreground & background colors are configured for some skins (see skin.prop files)
  • timestamp zero is always displayed when not playing, it's intentional. 2.0beta always displays a timestamp, but when not playing the value becomes erroneous after the first "repeat close" (could not try when playing, I can't make it work). Parsing the whole song is necessary to compute a correct timestamp, it's done when player starts but cannot be done permanently.
  • I hope it will work correctly on macOS, I have struggled to get something working correctly in all configs [Linux,Windows]*[SWT,JFX]

font for timestamp display is cuser-configurable (just click on timestamp)

tested successfully on:
- Linux openSuse tumbleweed, SWT & JFX
- Windows 10, SWT & JFX
- FreeBSD, SWT
built but not tested: FreeBSD, JFX
not built, not tested: MacOS, Android
Modification should have no impact on Android version

Most probably some adjustment will be needed. Possibly also bug fixes
still TODO:
- check if default font for timestamp is OK on MacOS (need a monospaced font)
- documentation update
@helge17
Copy link
Owner

helge17 commented Nov 4, 2024

@guiv42, I just cloned your toolbar branch. It seems to work fine on BSD and Linux and for me it looks good!

Also the JFX version works on MacOS (maybe I have to fine tune the font settings a little bit):
toolbar_jfx

But the SWT version freezes on MacoS with the following error:
toolbar_swt_error

% ./tuxguitar-2024-11-04-toolbar-macosx-swt-cocoa-x86_64.app/Contents/MacOS/tuxguitar.sh
java.lang.NullPointerException: Cannot read field "id" because the return value of "org.eclipse.swt.widgets.Control.paintView()" is null
	at org.eclipse.swt.widgets.Control.drawWidget(Control.java:1265)
	at org.eclipse.swt.widgets.Widget.drawRect(Widget.java:776)
	at org.eclipse.swt.widgets.Display.windowProc(Display.java:6043)
	at org.eclipse.swt.internal.cocoa.OS.objc_msgSendSuper(Native Method)
	at org.eclipse.swt.widgets.Display.applicationNextEventMatchingMask(Display.java:5284)
	at org.eclipse.swt.widgets.Display.applicationProc(Display.java:5716)
	at org.eclipse.swt.internal.cocoa.OS.objc_msgSend(Native Method)
	at org.eclipse.swt.internal.cocoa.NSApplication.nextEventMatchingMask(NSApplication.java:97)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3804)
	at org.herac.tuxguitar.ui.swt.SWTApplication.start(SWTApplication.java:64)
	at org.herac.tuxguitar.app.TuxGuitar.createUIContext(TuxGuitar.java:122)
	at org.herac.tuxguitar.app.TuxGuitar.createApplication(TuxGuitar.java:103)
	at org.herac.tuxguitar.app.TGMainSingleton.launchTuxGuitar(TGMainSingleton.java:57)
	at org.herac.tuxguitar.app.TGMainSingleton.launchSingleton(TGMainSingleton.java:45)
	at org.herac.tuxguitar.app.TGMainSingleton.main(TGMainSingleton.java:35)

Unfortunately, the MacOS code is a little outdated.

I also noticed that when playing a selection, the timestamp always starts at zero. Is that intentional?

@guiv42
Copy link
Collaborator Author

guiv42 commented Nov 4, 2024

Thanks for your feedback.

That's bad news for macOS: I can't debug this, and it seems the problem comes from SWT itself. I will try to downgrade SWT on Linux to see if I can reproduce, but I'm not very optimistic.
Result is a bit weird: several items are not drawn correctly (timestamp canvas, quarter icon for current tempo, icons for metronome and countdown), but tempo value and icon for playing mode are OK.

I also noticed that when playing a selection, the timestamp always starts at zero. Is that intentional?

I've seen this also. To be honest, no it wasn't intentional. But finally I think it makes sense. If you think it's better to display the absolute timestamp I can have a look.
I was also wondering: when not playing, it seems too complex to display a correct timestamp, especially because of repeats. In this case, what do you think is better: display "0:00:00.0" (as currently implemented), or just don't display anything?

@guiv42
Copy link
Collaborator Author

guiv42 commented Nov 5, 2024

I've had a very quick look at macOS/Linux SWT source codes, they are too different I will not reproduce.
So I think we have no choice but to upgrade SWT (#355)
@helge17 : could you please apply your patch, build with SWT4.26, and see what the toolbar looks like?
I'll try to refine analysis of #355.

@helge17
Copy link
Owner

helge17 commented Nov 5, 2024

Your toolbar branch works fine with SWT 4.26 on macOS!

toolbar_swt_4 26_ok

Should I apply the patch #355 (comment) to tuxguitar-next? It would also help me to upgrade my build system from macOS 11 to 14.

Maybe the patch #355 (comment) from 2.0beta helps to update the macOS code.

In this case, what do you think is better: display "0:00:00.0" (as currently implemented), or just don't display anything?

I think a black display is confusing, so I would prefer "0:00:00.0", or better "-:--:--.-"?

@guiv42
Copy link
Collaborator Author

guiv42 commented Nov 5, 2024

Your toolbar branch works fine with SWT 4.26 on macOS!

Good, thanks!

Should I apply the patch #355 (comment) to tuxguitar-next? It would also help me to upgrade my build system from macOS 11 to 14.

No, not yet. I agree we shall target this SWT upgrade, but it's a bit early. I'm currently analyzing #355, more details to come soon.

Maybe the patch #355 (comment) from 2.0beta helps to update the macOS code.

Unfortunately I don't think so.

I think a black display is confusing, so I would prefer "0:00:00.0", or better "-:--:--.-"?

You're right, "-:--:--.-" looks the best solution, I'll do this and update PR. Thanks!

@guiv42 guiv42 marked this pull request as draft November 5, 2024 17:11
@guiv42
Copy link
Collaborator Author

guiv42 commented Nov 5, 2024

PR #571 should handle the SWT update issue (#355).

Next step: I will replace the default timestamp when not playing and update this PR.
This PR can only be merged after PR #571

@guiv42
Copy link
Collaborator Author

guiv42 commented Nov 6, 2024

@helge17 : thank you for merging #571, it's not every day we succeed to tackle a macOS-specific issue :)
If you don't mind, I will also let you merge this one, as it deserves a minimum testing in macOS configuration (logically it should be OK)

@guiv42 guiv42 marked this pull request as ready for review November 6, 2024 17:56
@helge17 helge17 merged commit f77e392 into helge17:tuxguitar-next Nov 6, 2024
@guiv42 guiv42 deleted the toolbar branch November 6, 2024 20:27
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.

2 participants