Details
- Reviewers
elexis bb - Commits
- rP20226: Improve acoustic GUI notifications
- Trac Tickets
- #4101
#4609
Check if the current acoustical GUI notification (Nick notif.) worth correctly.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Build has FAILED
Link to build: http://jw:8080/job/phabricator/1895/
See console output for more information: http://jw:8080/job/phabricator/1895/console
| trunk/binaries/data/mods/public/gui/common/functions_utility.js | ||
|---|---|---|
| 6 ↗ | (On Diff #3259) | semicolomn |
| 14 ↗ | (On Diff #3259) | Imo it doesn't seem worth it to merge these objects so ok |
| 219 ↗ | (On Diff #3259) | when we add a sound for a buddy join, we will hear with this code the sound just once, not sure if that is intented, maybe we could set another object with the threshold times, which ofc can be set 0 in such a case. |
Ah, this patch indeed has two use cases incoming. Good to see!
| trunk/binaries/data/mods/public/gui/common/functions_utility.js | ||
|---|---|---|
| 14 ↗ | (On Diff #3259) | Doing so would make it obvious to every reader that there is always exactly one timer per sound file and vice versa. |
| 219 ↗ | (On Diff #3259) | magic number -> global |
| trunk/binaries/data/mods/public/gui/common/functions_utility.js | ||
|---|---|---|
| 219 ↗ | (On Diff #3259) | Meaning "when two buddies join simultaneously then only one sound" ^^ |
| trunk/binaries/data/mods/public/gui/common/functions_utility.js | ||
|---|---|---|
| 14 ↗ | (On Diff #3259) | We don't have to define the property in advance |
| 219 ↗ | (On Diff #3259) | g_Notifications = [
{
"soundfileorso": "filepath",
"threshold": 2000
},
{
"soundfileorso": "buddyjoin,
"threshold": possibly 0 as it's less likely to be noisespammed
}
];eventually the options dialog should become accessible globally (fpre has a patch for that somewhere iirc), |
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jenkins-master:8080/job/phabricator/2055/ for more details.
| binaries/data/mods/public/gui/common/functions_utility.js | ||
|---|---|---|
| 6 ↗ | (On Diff #3728) | old number seems to be 3000 but idc'ish |
| 213 ↗ | (On Diff #3728) | Notice this check is different now, since when Engine.ConfigDB_GetValue("user", "sound.notify." + type) == "false" it will still return false, since i guess we should return... (we were checking against a string) |
| binaries/data/mods/public/gui/gamesetup/gamesetup.js | ||
| 2237 ↗ | (On Diff #3728) | strictly could me inlined but meh |
| 2238 ↗ | (On Diff #3728) | remove newline while at it |
| 2240 ↗ | (On Diff #3728) | seems like ok to split rating |
| binaries/data/mods/public/gui/lobby/lobby.js | ||
| 1281–1282 ↗ | (On Diff #3728) | Why not merge ifs? |
| 1282 ↗ | (On Diff #3728) | (noticed: no rating here, so ok) |
| binaries/data/mods/public/gui/session/messages.js | ||
| 1109 ↗ | (On Diff #3728) | remove newline while at it |
| 1111 ↗ | (On Diff #3728) | Same tab => space here |
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jenkins-master:8080/job/phabricator/2056/ for more details.
No need for two globals. Use g_Notifications[foo].lastSomething instead of g_NotificationsTimes[foo].
| binaries/data/mods/public/gui/common/functions_utility.js | ||
|---|---|---|
| 5 ↗ | (On Diff #3734) | We have many notification types in the GUI (mostly in session/messages.js), so the name must be less ambiguous. Perhaps g_SoundNotifications or something like that |
| 209 ↗ | (On Diff #3734) | (should be self-evident but idc) |
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jenkins-master:8080/job/phabricator/2060/ for more details.
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jenkins-master:8080/job/phabricator/2061/ for more details.
Thanks for the update, code reads good to me. Tested the lobby session and gamesetup ping briefly and doesn't fall apart.
| binaries/data/mods/public/gui/common/functions_utility.js | ||
|---|---|---|
| 3 ↗ | (On Diff #3744) | (avoid spam) |
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jenkins-master:8080/job/phabricator/2065/ for more details.