Bug Hunting Session
Bug 48549 - System::Beep() removal
Summary: System::Beep() removal
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:3.7.0
Keywords: easyHack
Depends on:
Blocks:
 
Reported: 2012-04-11 04:39 UTC by Thomas Arnhold
Modified: 2015-12-22 01:40 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
Patch for fdo#48549 System::Beep() removal (26.61 KB, patch)
2012-07-19 11:30 UTC, Meko Otar
Details
Patch for fdo#48549 System::Beep() removal v2 (26.86 KB, patch)
2012-07-20 15:40 UTC, Meko Otar
Details
Patch for fdo#48549 System::Beep() removal v3 (35.81 KB, patch)
2012-07-29 22:04 UTC, Meko Otar
Details
patch removing unnecessary beep related methods (15.89 KB, patch)
2012-07-30 22:21 UTC, Meko Otar
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Arnhold 2012-04-11 04:39:13 UTC
The idea came up to remove all traces of System::Beep, which occurs randomly in the codebase.

To get the occurrences: git grep -5 Sound::Beep

Goal would be to remove it all.

Also see: http://lists.freedesktop.org/archives/libreoffice-ux-advise/2012-February/000888.html
Comment 1 Florian Reisinger 2012-05-18 08:07:09 UTC
Ok, good idea, but "easyhack" should be ini the whiteboard....
Comment 2 Meko Otar 2012-07-19 11:30:58 UTC
Created attachment 64387 [details]
Patch for fdo#48549 System::Beep() removal
Comment 3 Meko Otar 2012-07-19 11:34:44 UTC
I've written a patch that remove all call to Sound::Beep. There is still some warnings about unused parameters but I didn't wanted to change function prototypes.

I'm new to LibreOffice codebase so please give me some feedback.
Comment 4 Thomas Arnhold 2012-07-19 13:11:49 UTC
Hi Meko,

nice work! Looks good to me. Here are some unused parameters:

vcl/source/window/menu.cxx: The definition of nKeyCode is now unused (above your removal). So this else branch could go.

basctl/source/basicide/baside2.cxx: sal_Bool bRemoved = pLayout->GetWatchWindow().RemoveSelectedWatch();

only call for pLayout->GetWatchWindow().RemoveSelectedWatch(); without saving it, as bRemoved would be unused.


A question comes in mind: At some points the beep should say "here's an error". I've seen at least two points, where a debug message with OSL_FAIL() followed (sc/source/core/data/documen4.cxx). I think we should convert the beeps to debug messages at such points. Meko, what do you think? Maybe a post to the mailing list would be a good idea.

Thomas
Comment 5 Meko Otar 2012-07-20 15:40:44 UTC
Created attachment 64453 [details]
Patch for fdo#48549 System::Beep() removal v2
Comment 6 Meko Otar 2012-07-20 15:43:20 UTC
Hi, I've sent a new patch where I've removed the the two unused parameters mentioned by Thomas.
Comment 7 Not Assigned 2012-07-23 14:16:26 UTC
Mathieu Vonlanthen committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=0f6101cfef4c2e45d9f1f1b3a61ef94799e4526b

fdo#48549 System::Beep() removal
Comment 8 Thomas Arnhold 2012-07-25 17:35:31 UTC
Meko, there are some more beeps (you get them with `git grep -5 Sound::Beep`):

http://opengrok.libreoffice.org/xref/core/vcl/source/control/scrbar.cxx#988

In files were the Beep calls are gone, `#include <vcl/sound.hxx>` could go too.

Also there are approx. three calls in binfilter. If you want do do some more cleanup feel free to do so. Finally there should be no calls to Beep() and you could remove the Beep virtuals and implementations.
Comment 9 Thorsten Behrens (CIB) 2012-07-26 16:53:48 UTC
Comment on attachment 64453 [details]
Patch for fdo#48549 System::Beep() removal v2

2nd patch looks pushed as well. marking as obsolete to prevent it showing up in pending patch queries.
Comment 10 Meko Otar 2012-07-29 22:03:11 UTC
I've written a new patch cleaning remaining calls to Sound::Beep and unused include. I've also removed sound.hxx and sound.cxx as they were no more used.

I've built master with this patch without warnings on the modified files but there is still no logging added.

There is still calls to GetDisplay()->Beep() and gdk_display_beep(getGdkDisplay()), they are not directly related to fdo#48549 but they maybe deserve to be removed too?
Comment 11 Meko Otar 2012-07-29 22:04:33 UTC
Created attachment 64931 [details]
Patch for fdo#48549 System::Beep() removal v3
Comment 12 Thomas Arnhold 2012-07-30 03:17:46 UTC
Nice work! Pushed it. The remaining calls you noticed could be removed, as well as the corresponding implementations, meaning:

WinSalFrame::Beep
GtkSalFrame::Beep
X11SalFrame::Beep
SalDisplay::Beep
IosSalFrame::Beep
SvpSalFrame::Beep

and all the virtual declarations. Seems that vcl/sndstyle.hxx could go after that cleanup too.
Comment 13 Not Assigned 2012-07-30 03:25:45 UTC
Thomas Arnhold committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/binfilter/commit/?id=b4f09b1d25687bc648231f9fd52528d7fd6a053c

fdo#48549 Remove Sound::Beep
Comment 14 Meko Otar 2012-07-30 22:18:43 UTC
As Thomas Arnhold suggested, I've removed the following methods:
WinSalFrame::Beep
GtkSalFrame::Beep
X11SalFrame::Beep
SalDisplay::Beep
IosSalFrame::Beep
SvpSalFrame::Beep

I've also removed sndstyle.hxx and calls to MessageBeep().

I've done a complete build without warnings on modified files.

Thomas, if you know some similar tasks let me know.
Comment 15 Meko Otar 2012-07-30 22:21:06 UTC
Created attachment 64966 [details]
patch removing unnecessary beep related methods
Comment 16 Not Assigned 2012-07-31 09:33:02 UTC
Mathieu Vonlanthen committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=85cb9084533605657aca0394afe4516058a8e4ef

fdo#48549 System::Beep() removal
Comment 17 Thomas Arnhold 2012-07-31 09:34:17 UTC
Comment on attachment 64966 [details]
patch removing unnecessary beep related methods

Awesome! I let you know :)
Comment 18 Rainer Bielefeld Retired 2013-02-24 07:43:57 UTC
I asked for a review concerning ACCESSIBILITY impact on <accessibility@global.libreoffice.org>
Comment 19 V Stuart Foote 2013-02-24 17:16:54 UTC
Not an issue for ACCESSIBLITY, where for Assistive Technology (AT) we should not be directly generating audible queues for input or alert events. There are specific XAccessible roles for that purpose in UNO AAPI that is the correct way to interact with AT.  Other sounds by LibreOffice are disruptive.
Comment 20 Robinson Tryon (qubit) 2015-12-22 01:40:58 UTC
Migrating Whiteboard tags to Keywords: (easyhack)
[NinjaEdit]