Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ if (BRICKMAN_TEST)
test/FakeBluez5.vala
test/FakeConnman.vala
test/controller/FakeAboutController.vala
test/controller/FakeAudioController.vala
test/controller/FakeBatteryController.vala
test/controller/FakeBluetoothController.vala
test/controller/FakeDeviceBrowserController.vala
Expand All @@ -52,12 +53,14 @@ else (BRICKMAN_TEST)
lib/connman/Service.vala
lib/connman/Technology.vala
src/controller/AboutController.vala
src/controller/AudioController.vala
src/controller/BatteryController.vala
src/controller/BluetoothController.vala
src/controller/DeviceBrowserController.vala
src/controller/FileBrowserController.vala
src/controller/NetworkController.vala
src/controller/OpenRobertaController.vala
src/AlsaBackedMixerElement.vala
src/GlobalManager.vala
src/main.vala
)
Expand All @@ -71,6 +74,7 @@ set (BRICKMAN_COMMON_SOURCE_FILES
lib/systemd/Systemd.vala
lib/systemd/logind-interfaces.vala
lib/systemd/systemd-interfaces.vala
src/AlsaInterface.vala
src/controller/IBrickManagerModule.vala
src/dbus/Bluez5Agent.vala
src/dbus/ConnmanAgent.vala
Expand All @@ -86,6 +90,8 @@ set (BRICKMAN_COMMON_SOURCE_FILES
src/view/DeviceBrowserWindow.vala
src/view/FileBrowserWindow.vala
src/view/HomeWindow.vala
src/view/MixerElementSelectorWindow.vala
src/view/MixerElementVolumeWindow.vala
src/view/MotorBrowserWindow.vala
src/view/MotorInfoWindow.vala
src/view/MotorValueDialog.vala
Expand Down Expand Up @@ -118,6 +124,7 @@ set (BRICKMAN_COMMON_SOURCE_FILES

find_package(PkgConfig REQUIRED)
pkg_check_modules(DEPS REQUIRED
alsa
ev3devkit-0.4
glib-2.0
gobject-2.0
Expand Down Expand Up @@ -174,6 +181,7 @@ PACKAGES
curses
posix
linux
alsa
${BRICKMAN_TEST_PACKAGES}
CUSTOM_VAPIS
bindings/*.vapi
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ not in brickstrap shell:

mkdir -p <some-build-dir>
cd <some-build-dir>
cmake <path-to-brickdm-source> -DCMAKE_BUILD_TYPE=string:Debug -DBRICKMAN_TEST=bool:Yes
cmake <path-to-brickman-source> -DCMAKE_BUILD_TYPE=string:Debug -DBRICKMAN_TEST=bool:Yes
make
make run

Expand Down
3 changes: 2 additions & 1 deletion debian/control
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ Priority: standard
Maintainer: David Lechner <david@lechnology.com>
Build-Depends: debhelper (>= 9), dh-systemd, cmake, valac (>= 0.24),
libgirepository1.0-dev, libgudev-1.0-dev,
libncurses5-dev, libev3devkit-dev, netpbm
libncurses5-dev, libev3devkit-dev, netpbm,
libasound2-dev
Standards-Version: 3.9.5
Homepage: https://www.ev3dev.org
Vcs-Git: git://github.com/ev3dev/brickman.git
Expand Down
105 changes: 105 additions & 0 deletions src/AlsaBackedMixerElement.vala
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
/*
* brickman -- Brick Manager for LEGO MINDSTORMS EV3/ev3dev
*
* Copyright (C) 2016 Kaelin Laundry <wasabifan@outlook.com>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
* MA 02110-1301, USA.
*/

/* AlsaBackedMixerElement.vala - Implementation of IMixerElementViewModel using ALSA API */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to have view models now, let's start a new subdirectory for them., namely `view_model, and put this file in it.


using Alsa;

namespace BrickManager {
public class AlsaBackedMixerElement: IMixerElementViewModel, Object {
private const SimpleChannelId primary_channel_id = SimpleChannelId.MONO;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning on changing this value? I don't see a need to. So, we don't really need a constant for it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the equivalent of a "magic number" and is used in two places, so having it as a constant makes the code more readable and maintainable. While I don't intend to change it, I already had to change it a few times in development, so there's no reason to think I wouldn't need to change it again.


private unowned MixerElement alsa_element;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for private keyword. It is implied.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to be explicit whenever there's the possibility for confusion; see here and here. It generally makes it easier for others to read and undestand the code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also something to be said for being consistent. We don't use private anywhere else in brickman (or at least not much).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I can change that to keep it consistent.

private SimpleElementId alsa_id;

public AlsaBackedMixerElement(MixerElement element) {
this.alsa_element = element;
SimpleElementId.alloc(out alsa_id);
element.get_id(alsa_id);
}

public string name {
get {
return alsa_id.get_name();
}
}

public uint index {
get {
return alsa_id.get_index();
}
}

public int volume {
get {
long volume = 0;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need to initialize value when used as out parameter.

long volume;

will do.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's an out parameter, if the code that I call doesn't set it, it will be left at whatever value it had before right? So I have it this way to ensure it's a known value if an error occurs and their code doesn't set a value.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, in this case, you should check for an error and set the value if reading volume after an error is valid. I also just noticed that the local variable is the same name as the property, which is not ideal.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still like to see error checking here.

alsa_element.get_playback_volume(primary_channel_id, out volume);

long min_volume, max_volume;
alsa_element.get_playback_volume_range(out min_volume, out max_volume);

// Prevent division by zero
if(max_volume == min_volume)
return 0;

// Do calculations as floats so avoid odd-looking increments from truncation
return (int)Math.round((volume - min_volume) * 100 / (float)(max_volume - min_volume));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simplify this by using alsa_element.set_playback_volume_range(0, 100), then we can just write the value directly instead of scaling it ourselves. Haven't actually tried this out though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works, then calling alsa_element.set_playback_volume_range() would be more appropriate in the constructor.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this now and it works very nicely.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This originally seemed like a great approach, however it isn't so good when you consider the fact that other apps can also set the range. So, if we did it this way, the get wouldn't work after using alsamixer (or another app) or the first time that brickman was used. At that point, these changes would only simplify the set portion; so it would add a call to set the range but remove the math on the next line. That seems like a wash to me.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I'm actually working on another app that pokes the alsa mixer, so this is good to know.

Copy link
Copy Markdown
Member

@dlech dlech Jun 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, I don't think using float has any benefit here. You are still truncating with Math.round(). As long as you multiply before dividing, you will get the exact same result.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing I just discovered is that if you call snd_mixer_selem_set_playback_volume_all (MixerElement.set_playback_volume_all in Vala), to the same value as the previous call, it does not actually change the mixer value if another mixer program has changed the value. I don't know that it is a problem with the code here, but just an interesting observation.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, I don't think using float has any benefit here. You are still truncating with Math.round() . As long as you multiply before dividing, you will get the exact same result.

Unfortunately, doing it all as integer math (and multiplying before dividing) doesn't give the same result. I added the floats because both the write and the read were rounding down slightly through truncation so that the even increments on the 100 scale turned into inconsistent increments after the value was set and read. For example, going volume up then volume down repeatedly would continuously decrease the volume because of the rounding.

Another thing I just discovered...

Interesting. I don't think there is much we can do about that, but it's good to know in case that turns into a problem later.

}
set {
long min_volume, max_volume;
alsa_element.get_playback_volume_range(out min_volume, out max_volume);

int constrained_volume = int.min(100, int.max(0, value));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer var for variable declarations

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

float scaled_volume = constrained_volume * (max_volume - min_volume) / 100f + min_volume;
long rounded_volume = (long)Math.round(scaled_volume);

alsa_element.set_playback_volume_all(rounded_volume);

bool should_mute = rounded_volume <= min_volume;
if(is_muted != should_mute)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: space after if and {} around body.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will-do

set_is_muted(should_mute);
}
}

public bool can_mute {
get {
return alsa_element.has_playback_switch();
}
}

public bool is_muted {
get {
if(!can_mute)
Copy link
Copy Markdown
Member

@dlech dlech Jun 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style (see previous comment)

return false;

int mute_switch = 0;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, no need to initialize.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above; although in this case, it should actually be inverted. I'll switch that to a 1.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

theoretically, if there is an error, the value could be changed - the state is undefined, so better to check for error and set value if error.

alsa_element.get_playback_switch(primary_channel_id, out mute_switch);

return mute_switch == 0;
}
}

protected void set_is_muted(bool is_muted) {
Copy link
Copy Markdown
Member

@dlech dlech Jun 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this not a setter of the is_muted property?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't add a setter to the property because the interface defines it as get-only. So, separate setters. This behavior is different from that of C# and friends.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the setters are public, why not just add set to the interface?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is protected because it's only for internal use, and other implementations of the interface won't necessarily need a setter for mute.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I was confusing this with the FakeMixerElement

if(can_mute)
alsa_element.set_playback_switch_all(is_muted ? 0 : 1);
}
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newline at end of file

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will-do

110 changes: 110 additions & 0 deletions src/AlsaInterface.vala
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/*
* brickman -- Brick Manager for LEGO MINDSTORMS EV3/ev3dev
*
* Copyright (C) 2016 Kaelin Laundry <wasabifan@outlook.com>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
* MA 02110-1301, USA.
*/

/* AlsaInterface.vala - Definitions for interfacing with ALSA */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with this file. It should go in the view_model subdirectory.


using Alsa;

namespace BrickManager {
public interface IMixerElementViewModel : Object {
public const int MIN_VOLUME = 0;
public const int MAX_VOLUME = 100;
public const int HALF_VOLUME = (MAX_VOLUME + MIN_VOLUME) / 2;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be - instead of +?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm taking the mean, so it sums the values then devides by the number of values.


public abstract string name { get; }
public abstract uint index { get; }
public abstract int volume { get; set; }
public abstract bool can_mute { get; }
public abstract bool is_muted { get; }
}

public class FakeMixerElement: IMixerElementViewModel, Object {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this class should go in a separate file under the test subdirectory so that it is not compiled in the "regular" version of Brickman.

private string _name;
private uint _index;
private int _volume;
private bool _can_mute;
private bool _is_muted;

public string name {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not public string name { get; private set; } instead?

Or if you want to be really glib-ish, public string name { get; construct; }

Copy link
Copy Markdown
Member

@dlech dlech Jun 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nix the construct, I see now you have setters below. So, this should just be

public string name { get; set; }

There is no need for separate setter functions (unless they throw an error that you want to catch).

Also, there is no need to call notify_property in the setter. This is done automatically in GLib.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above note; can't have a set clause because the interface defines it as get-only.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does public override string name { get; set; } work?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, forgot to reply to this one. Adding override does not resolve the problem; there isn't anything to override because the types don't match after adding a set.

get {
return _name;
}
}

public uint index {
get {
return _index;
}
}

public FakeMixerElement(string name, uint index, int volume, bool can_mute) {
set_name(name);
set_index(index);
this.volume = volume;

set_can_mute(can_mute);
}

public int volume {
get {
return _volume;
}
set {
_volume = int.min(100, int.max(0, value));

bool should_mute = _volume <= 0;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

volume won't be < 0 here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of those "intent" things; I do that to make it clear that the intent is to check if the volume is not at an audible level. It just makes the code easier to read at a glance, and more resilient if changes are made later.

if(_is_muted != should_mute)
set_is_muted(should_mute);
}
}

public bool can_mute {
get {
return _can_mute;
}
}
public bool is_muted {
get {
return _is_muted;
}
}

public void set_name(string new_name) {
this._name = new_name;
notify_property("name");
}

public void set_index(uint new_index) {
this._index = new_index;
notify_property("index");
}

public void set_can_mute(bool can_mute) {
this._can_mute = can_mute;
notify_property("can_mute");
}

public void set_is_muted(bool is_muted) {
this._is_muted = is_muted;
notify_property("is_muted");
}
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newline at end of file

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

99 changes: 99 additions & 0 deletions src/controller/AudioController.vala
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* brickman -- Brick Manager for LEGO MINDSTORMS EV3/ev3dev
*
* Copyright 2014-2015 David Lechner <david@lechnology.com>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't write this. 😄

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, thought I got them all; looks like I completely forgot to change this header.

*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
* MA 02110-1301, USA.
*/

/* BatteryController.vala - Controller for monitoring battery */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not BatteryController


using Ev3devKit.Devices;
using Ev3devKit.Ui;
using Alsa;

namespace BrickManager {
public class AudioController : Object, IBrickManagerModule {
private const int VOLUME_STEP = 10;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for private

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above


Mixer mixer;
MixerElementSelectorWindow mixer_select_window;
MixerElementVolumeWindow volume_window;

public string display_name { get { return "Sound"; } }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if the file name matched the display name. i.e. SoundController

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will-do


protected void initialize_mixer() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this protected?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's something that any future subclasses should be able to use. At a minimum, it illustrates intent, but in the future it may actually be used.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this ever being subclassed. I would consider all of the controller classes sealed in C# speak. But if it does ever get subclassed, you can always change it to protected if needed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly about it either way; I'll just default it to private and it can be dealt with later if it's ever subclassed.

mixer = null;
Mixer.open(out mixer);
Copy link
Copy Markdown
Member

@dlech dlech Jun 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be checking error values here in case something goes wrong. At a minimum:

err = Mixer.open (out mixer);
if (err < 0) {
    critical ("Opening mixer failed: %s", strerror (err));
    return;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will-do.

mixer.attach();
mixer.register();
mixer.load();
}

void create_main_window () {
mixer_select_window = new MixerElementSelectorWindow ();

mixer_select_window.mixer_elem_selected.connect ((selected_element) => {
if(volume_window == null)
create_volume_window();

volume_window.current_element = selected_element;
volume_window.show_element_details = true;
volume_window.show();
});
}

void create_volume_window() {
volume_window = new MixerElementVolumeWindow();

// Wire up handlers for volume window signals
volume_window.volume_up.connect(() =>
volume_window.current_element.volume += VOLUME_STEP);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reference cycle. Need to use a weak reference to volume_window in the body of the lambda statement.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... I must not understand reference cycles well enough. What makes this a reference cycle? Also, why does this not cause the program to crash/freeze?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A reference cycle leads to a memory leak. https://wiki.gnome.org/Projects/Vala/ReferenceHandling

So what happens here is that in the generated C code, it takes a reference to volume_window in the lambda expression and creates a closure structure. That closure is assigned to volume_window when the signal is connected, so now volume_window has a reference to itself and will never be freed.

So, you have to break this reference cycle. You could do this by disconnecting the signal, for example when the window is closed.

You can also do it by using a weak reference to volume_window. This way, when the C closure is created, it does not take a reference to volume_window. Since the signal will be disconnected when volume_window is destroyed, you don't have to worry about the signal being called after volume_window is freed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I get it -- so it's not actively increasing the reference count in a cycle, it just holds a reference that will never let it be freed. I'll fix that.


volume_window.volume_down.connect(() =>
volume_window.current_element.volume -= VOLUME_STEP);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reference cycle


volume_window.volume_min.connect(() =>
volume_window.current_element.volume = IMixerElementViewModel.MIN_VOLUME);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reference cycle

}

public void show_main_window () {
if (mixer_select_window == null) {
create_main_window ();
}

// Whenever the audio item is launched from the main menu,
// repopulate the mixer list
mixer_select_window.clear_elements();
// Re-initializing will return updated data, including volume
initialize_mixer();
for(MixerElement element = mixer.first_elem(); element != null; element = element.next()) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space between for and (

mixer_select_window.add_element(new AlsaBackedMixerElement(element));
}

if(mixer_select_window.has_single_element) {
if(volume_window == null)
create_volume_window();

volume_window.current_element = mixer_select_window.first_element;
volume_window.show_element_details = false;
volume_window.show();
}
else
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: cuddled else. i.e.} else {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh gosh, you're a monster! 😁 I'll make that change.

mixer_select_window.show ();
}
}
}
Loading