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

Let beep use system default settings #243

Merged
merged 3 commits into from
Jul 16, 2022
Merged

Let beep use system default settings #243

merged 3 commits into from
Jul 16, 2022

Conversation

lwi
Copy link
Contributor

@lwi lwi commented Jun 25, 2021

Please note that the parameter 'percent' of XBell(3).
Given the value of 100, the system defined setting
(normally specified via xset) is ignored and the
percent of 100 is used instead. When calling the
bell from fltk with FL_BEEP_DEFAULT I would expect
to get the default (system specified) percent setting.

Please note that the parameter 'percent' of XBell(3).
Given the value of 100, the system defined setting
(normally specified via xset) is ignored and the
percent of 100 is used instead. When calling the
bell from fltk with FL_BEEP_DEFAULT I would expect
to get the default (system specified) percent setting.
@Albrecht-S
Copy link
Member

Thanks for the report and the patch. However, the patch is IMHO not correct (see below) and I wonder on what platform you (@lwi) are working. Some thoughts:

  • I can't hear any sound when I execute fl_beep() or XBell() on my Linux laptop (Mint 20, based on Ubuntu 20.04). There is a note in commit bed8043 (from Dec 2005!): "We have to support ALSA on Linux because the current Xorg releases no longer support XBell() or the PC speaker". If this is true since 2005 I'm not surprised that fl_beep() is silent on my system. Hence I wonder on which X system you are working. Can you tell us details? Is this not using the Xorg server? What bell/beep/sound settings do you have, and are you hearing the sound from the system speaker or from your sound card?

  • the FLTK function fl_beep() has no way to specify the sound volume as you may have noticed. The given argument (e.g. FL_BEEP_DEFAULT) specifies the sound type, not the sound volume. Given the current code, FL_BEEP_DEFAULT and FL_BEEP_ERROR share the same value (100) whereas all other enum values use 50 which seems to indicate that the original author(s) tried to distinguish different sound types by sound volume on X11 since the system function XBell() was not able to "play" different sounds (like on the Windows platform).

  • the given patch (PR) is IMHO wrong (kinda incomplete) because it would (only) affect FL_BEEP_DEFAULT and FL_BEEP_ERROR but leave all other enum values using volume level 50 which would be louder than FL_BEEP_ERROR.

  • note that I read and understood the algorithm given in the XBell() man page to determine the sound level and I agree with you that the value 100 uses the highest possible sound volume and ignores the user setting (if any)

See followup for possible solutions...

@Albrecht-S
Copy link
Member

Albrecht-S commented Jun 25, 2021

That all said, what can we do?

One possible solution would be to do as you (the OP, @lwi) suggested for FL_BEEP_ERROR which should obviously be the loudest beep generated by fl_beep() - as intended by the original author(s). Setting the value to 0 as proposed would honor the user setting which is IMHO good behavior.

But then we'd need to set all other values to a lower volume level which means we need to use a negative value in XBell(), for instance:

  • FL_BEEP_DEFAULT: either 0 (same as FL_BEEP_ERROR) or a slightly lower value like -10 or -20
  • all others: a value lower than FL_BEEP_DEFAULT, maybe FL_BEEP_DEFAULT - 10 or FL_BEEP_DEFAULT - 20

What do you think about such a modified solution?

Here's a possible implementation (file src/drivers/X11/Fl_X11_Screen_Driver.cxx, lines 412-428):

void Fl_X11_Screen_Driver::beep(int type)
{
  int vol = 0;
  switch (type) {
    case FL_BEEP_ERROR :
      vol = 0;
      break;
    case FL_BEEP_DEFAULT :
      vol = -10;
      break;
    default :
      vol = -20;
      break;
  }
  if (!fl_display) open_display();
  XBell(fl_display, vol);
}

As I wrote before I can't test this currently because I don't hear any sound when using fl_beep() on my system. Would you be willing to test this on your system? Note that you can copy and paste this code to try it on your system.

Thanks in advance.

@Albrecht-S Albrecht-S self-assigned this Jun 25, 2021
@Albrecht-S Albrecht-S added the enhancement New feature or request label Jun 25, 2021
@lwi
Copy link
Contributor Author

lwi commented Jun 25, 2021

Thanks for your reply.

@lwi lwi closed this Jun 25, 2021
@Albrecht-S
Copy link
Member

Why did you close this? I think that it would be an enhancement, at least on systems that (still) use the PC speaker and have a working XBell() function.

@lwi
Copy link
Contributor Author

lwi commented Jun 25, 2021

I pressed the wrong button :/ Will write detailed response shortly.

@lwi lwi reopened this Jun 25, 2021
@lwi
Copy link
Contributor Author

lwi commented Jun 25, 2021

We are running Debian testing + X11. We are using the pc speaker for decades, on some hardware we even manually retro-fit a pc speaker. I cannot confirm that the XBell stopped working at any point in time. We have the bell percent set as 50 system-wide as default. When using tigervnc client (which relies on fltk) we realized that the beep was "too loud" and I traced it back to fltk which uses XBell with parameter 100 since '99 unfortunately. But similar to your writing I assume the 50/100 was meant to indicate different loudness for different severity.
I would however propose to use the default system value for FL_BEEP_DEFAULT. In our use case the vnc client uses FL_BEEP_DEFAULT for "forwarding the beep event". In my opinion this "translation" should be passed forward unchanged.
For an error beep one could argue fltk should pick the loudest beep there is (percent=100).
I agree my PR is incomplete in the sense that I only change the default-case but not the other.
Are you sure your hardware has a PC speaker built-in? Modern hardware tend not to have that anymore. There are some other software packages around (one is aptly called "beep") you might use to test this "useful" hardware feature ;)

@lwi
Copy link
Contributor Author

lwi commented Jun 25, 2021

Original commit to introduce parameter value 100 is ef8eaf2 for the historically curious.

@Albrecht-S
Copy link
Member

Short reply (details later): feel free to modify my "patch" with your suggested values...

@lwi
Copy link
Contributor Author

lwi commented Jun 25, 2021

Looks pretty clean this way. One could of course very shortly go:
XBell(fl_display, type == FL_BEEP_ERROR? 100: 0)
but that surely does not increase readability.

@lwi
Copy link
Contributor Author

lwi commented Jul 12, 2021

Any further objections the proposed approach? (ping)

@Albrecht-S
Copy link
Member

No, basically the patch is fine. I've been working on documentation enhancements but got stuck and have been busy with other stuff. I'll come back to this issue hopefully soon, as time permits.

@lwi
Copy link
Contributor Author

lwi commented Nov 26, 2021

ping ;)

@Albrecht-S Albrecht-S added this to the Release FLTK 1.4.0 milestone Nov 26, 2021
@Albrecht-S
Copy link
Member

pong ;)

It's not forgotten and will definitely be in 1.4.0 (I added it right now to our release milestone)

@Albrecht-S Albrecht-S merged commit 3560ff4 into fltk:master Jul 16, 2022
@Albrecht-S
Copy link
Member

Merged. Sorry for the long delay. Thanks for the PR.

FTR: I wanted to squash the commits but something went awry. Anyway, it's merged now.

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.

None yet

2 participants