2017-02-23 13:13 GMT+01:00 <[email protected]>: > On Thu, Feb 23, 2017 at 01:00:10PM +0100, michael bouchaud wrote: > > 2017-02-23 12:37 GMT+01:00 <[email protected]>: > > > > > On Thu, Feb 23, 2017 at 12:14:05PM +0100, michael bouchaud wrote: > > > > 2017-02-23 12:01 GMT+01:00 <[email protected]>: > > > > > > > > > First of all to the technical side: > > > > > > > > > > The commit i pushed removes the code that fetches the volume from > pa > > > > > when the drag of a slider is done. Reason for that was that we > might > > > > > set a old volume from pulseaudio which is not the last one the user > > > > > dragged to. This can happen bacause the volume is sometimes not up > to > > > > > date when we are ending the drag, if pa gives us a new volume that > is > > > > > not exact to the one we had when the drag stopped (for example some > > > > > clamping of the volume happened inside pulseaudio) then we are > getting > > > > > a callback where we are setting the volume again in the slider. So > > > > > that removing of code does not really remove a feature. We are > getting > > > > > notified later about a new volume. > > > > > > > > > > > > > No we aren't notified, because the volume doesn't change. If the > volume > > > > is setted to 100 as example and you set it to 105. emix won't change > the > > > > volume. The volume is still setted to 100 not 105 ... > > > > > > Well depends on what you are doing. If you are dragging then it should > > > work, if you do it with clicking/keybindings then it will clamp at 100% > > > (BARRIER > > > feature),. Can you elaborate a bit on your usecase? > > > > > > > you have 100 setted and you drag to 105, as you say BARRIER feature > > won't change the volume. But without this commit the slider stay at 105. > > It did before commit dbdf411b488fd4d3f37a26d8cb142b25aba784d6. Reread > the patch, you are removing there a elm_slider_value_set, this ensured > that the value stayed at the value returned by the barrier feature. > > > > > > > > > > > > > > > > > > > > > > > On Thu, Feb 23, 2017 at 11:30:18AM +0100, Michaël Bouchaud wrote: > > > > > > Sorry for the miss of comments about this revert. > > > > > > > > > > > > I explained to you, this commit remove something needed to get > proper > > > > > value > > > > > > setted to the volume slider. > > > > > > > > > > What is that something ? As explained above, if something else > changes > > > > > the volume again, we are getting a callback that results in a > update of > > > > > the slider. > > > > > > > > > > > When you have done this revert you have posted this video > > > > > > https://omicron.homeip.net/filedump/mixer_gadget_bug.ogv. We > can see > > > > > that > > > > > > emixer works great but not the enlightenment mixer module. Both > share > > > > > about > > > > > > 90 > > > > > > percents of the code to talk to pulseaudio. So no need to talk > with > > > > > > pulseaudio > > > > > > guys, we got something weird into our enlightenment volume > mixer. I > > > > > analyze > > > > > > code of two and see the enlightenment module make some alloc to > set > > > the > > > > > > volume > > > > > > and emixer doesn't. I fix that in the previous commit > > > > > > (dbdf411b488fd4d3f37a26d8cb142b25aba784d6). I've asked > morlenxus to > > > > > check my > > > > > > branch, but he never came back to say if this work. I don't see > why > > > this > > > > > > wouldn't working because emixer and enlightenment module are the > same > > > > > code > > > > > > now. > > > > > > > > > > While analyzing the code of emixer.c and the module you probebly > should > > > > > have noticed that emixer doesnt do exactly THAT. it does not set > the > > > > > volume again on drag end. thats NOT done. The only place where > this is > > > > > done is on a input volume slider, not the sink or sink input. > > > > > And the bug in the video is clearly happening for sink / sink > input. > > > > > > > > > > > > > ??? Sorry but the code is in the revert ... > > > > > > > > > > Your commit (fba185798cf75eaeaba4a95d2be25fb2fea6ef1a) brings back the > > > resetting of the volume on drag stop. Show me where this is done in > > > emixer.c for a slider of the sink volume. > > > > > > > You remove it ... so :) > > yeah. in e_mod_main.c but since you are comparing emixer with the module > you should also have the code you want in somewhere in emixer.c show me > where it is. > > > > > > > > > > > > > > > > > > > > The weirdness part in our enlightenment codebase here is that we > > > > > sometimes get into the state where we are getting updates from > > > > > pulseaudio very slowly (Can be caused by havy work load, etc. > etc.). > > > > > > > > > > > > > Absolutely not, we are just rereading the value we have setted to > > > > pulseaudio. > > > > Pulseaudio isn't solicited at this time. > > > > > > > > > > Its not like this, read the pulse code of emix. if we are setting a > > > volume to pulseaudio it just translates it to a pulseaudio volume > struct > > > and sets it for that sink. It never attaches the same volume struct to > > > the sink. The volume in the sink is only updated when pa sends the > > > callback of "hey volume has changed" (pulse.c:215) > > > > > > > yes but the code here don't call any emix or pulse stuff. Just reread the > > value. > > > > /* retrieve data */ > > EINA_SAFETY_ON_NULL_RETURN(mixer_context->sink_default); > > Emix_Sink *s = (Emix_Sink *)mixer_context->sink_default; > > /* Read it */ > > int val = s->volume.volumes[0]; > > /* Set it */ > > elm_slider_value_set(obj, val); > > > > Please if I'm wrong, spot me where we call pulse stuff. > > No it does not. s->volume.volumes[0] is still the old last value known > from pulseaudio. Not the value we have setted. And this is bad and can > result in the bug you can see in the video. >
Not the value setted to pulseaudio ? And what do VOLSET macro ? > > > > > > > > > But now, lets assume you are right. That means that we are setting > EXACTLY > > > the same value we have just gotten from our slider, back to our slider. > > > Why is that needed? > > > > > > > To be sure we display the correct value setted to pulse (BARRIER feature > > could not change the volume or set it to a different value). > > > > If you want to see why we need this. Open emixer and move the > > enlightenment module slider in different value. You will see we don't get > > the same value on both in some case. > > > > > > > > > > > > > > > > > > > > > > And more important, not getting this commit into our code just > hidde > > > us > > > > > the > > > > > > bug > > > > > > and give us no chance to spot it. > > > > > > > > > > Thats a part i dont understand :) > > > > > > > > > > > > > > > > > > > > > > > 2017-02-23 9:09 GMT+01:00 Marcel Hollerbach < > > > > > [email protected]>: > > > > > > > > > > > > > bu5hm4n pushed a commit to branch master. > > > > > > > > > > > > > > http://git.enlightenment.org/core/enlightenment.git/commit/ > ?id= > > > > > > > 9745890a37036091d5dec320fecda7ed4c6bdb6c > > > > > > > > > > > > > > commit 9745890a37036091d5dec320fecda7ed4c6bdb6c > > > > > > > Author: Marcel Hollerbach <[email protected]> > > > > > > > Date: Thu Feb 23 09:08:24 2017 +0100 > > > > > > > > > > > > > > Revert "Revert "mixer: do not set back the value from emix > > > once the > > > > > > > drag is finished"" > > > > > > > > > > > > > > This reverts commit fba185798cf75eaeaba4a95d2be25f > b2fea6ef1a. > > > > > > > > > > > > > > There is not even a description why you reverted it. This > is a > > > > > bugfix > > > > > > > that fixed a bug. So talk to me what the issue is, but > please > > > stop > > > > > > > reverting commits silently. > > > > > > > --- > > > > > > > src/modules/mixer/e_mod_main.c | 11 ----------- > > > > > > > src/modules/mixer/emixer.c | 13 ------------- > > > > > > > 2 files changed, 24 deletions(-) > > > > > > > > > > > > > > diff --git a/src/modules/mixer/e_mod_main.c > > > b/src/modules/mixer/e_mod_ > > > > > > > main.c > > > > > > > index ac805cc..2c86915 100644 > > > > > > > --- a/src/modules/mixer/e_mod_main.c > > > > > > > +++ b/src/modules/mixer/e_mod_main.c > > > > > > > @@ -481,16 +481,6 @@ _slider_changed_cb(void *data EINA_UNUSED, > > > > > > > Evas_Object *obj, > > > > > > > } > > > > > > > > > > > > > > static void > > > > > > > -_slider_drag_stop_cb(void *data EINA_UNUSED, Evas_Object *obj, > > > > > > > - void *event EINA_UNUSED) > > > > > > > -{ > > > > > > > - EINA_SAFETY_ON_NULL_RETURN(mixer_context->sink_default); > > > > > > > - Emix_Sink *s = (Emix_Sink *)mixer_context->sink_default; > > > > > > > - int val = s->volume.volumes[0]; > > > > > > > - elm_slider_value_set(obj, val); > > > > > > > -} > > > > > > > - > > > > > > > -static void > > > > > > > _sink_selected_cb(void *data, Evas_Object *obj EINA_UNUSED, > void > > > > > > > *event_info EINA_UNUSED) > > > > > > > { > > > > > > > Emix_Sink *s = data; > > > > > > > @@ -554,7 +544,6 @@ _popup_new(Instance *inst) > > > > > > > evas_object_show(slider); > > > > > > > elm_slider_min_max_set(slider, 0.0, emix_max_volume_get()); > > > > > > > evas_object_smart_callback_add(slider, "changed", > > > > > _slider_changed_cb, > > > > > > > NULL); > > > > > > > - evas_object_smart_callback_add(slider, "slider,drag,stop", > > > > > > > _slider_drag_stop_cb, NULL); > > > > > > > elm_slider_value_set(slider, volume); > > > > > > > elm_box_pack_end(bx, slider); > > > > > > > evas_object_show(slider); > > > > > > > diff --git a/src/modules/mixer/emixer.c > > > b/src/modules/mixer/emixer.c > > > > > > > index 5cde881..1bcd96c 100644 > > > > > > > --- a/src/modules/mixer/emixer.c > > > > > > > +++ b/src/modules/mixer/emixer.c > > > > > > > @@ -49,17 +49,6 @@ _cb_sink_volume_change(void *data, > > > > > > > } > > > > > > > > > > > > > > static void > > > > > > > -_cb_sink_volume_drag_stop(void *data, > > > > > > > - Evas_Object *obj, > > > > > > > - void *event EINA_UNUSED) > > > > > > > -{ > > > > > > > - Evas_Object *bxv = data; > > > > > > > - Emix_Sink *sink = evas_object_data_get(bxv, "sink"); > > > > > > > - int vol = sink->volume.volumes[0]; > > > > > > > - elm_slider_value_set(obj, vol); > > > > > > > -} > > > > > > > - > > > > > > > -static void > > > > > > > _cb_sink_mute_change(void *data, > > > > > > > Evas_Object *obj, > > > > > > > void *event_info EINA_UNUSED) > > > > > > > @@ -134,8 +123,6 @@ _emix_sink_add(Emix_Sink *sink) > > > > > > > elm_box_pack_end(bx, sl); > > > > > > > evas_object_show(sl); > > > > > > > evas_object_smart_callback_add(sl, "changed", > > > > > _cb_sink_volume_change, > > > > > > > bxv); > > > > > > > - evas_object_smart_callback_add(sl, "slider,drag,stop", > > > > > > > - _cb_sink_volume_drag_stop, > bxv); > > > > > > > > > > > > > > ck = elm_check_add(win); > > > > > > > evas_object_data_set(bxv, "mute", ck); > > > > > > > > > > > > > > -- > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Michaël Bouchaud (yoz) <[email protected]> > > > > > > ------------------------------------------------------------ > > > > > ------------------ > > > > > > Check out the vibrant tech community on one of the world's most > > > > > > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > > > > > > _______________________________________________ > > > > > > enlightenment-devel mailing list > > > > > > [email protected] > > > > > > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > > > > > > > > > > ------------------------------------------------------------ > > > > > ------------------ > > > > > Check out the vibrant tech community on one of the world's most > > > > > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > > > > > _______________________________________________ > > > > > enlightenment-devel mailing list > > > > > [email protected] > > > > > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > > > > > > > > > > > > > > > > > > > > > -- > > > > Michaël Bouchaud > > > > ------------------------------------------------------------ > > > ------------------ > > > > Check out the vibrant tech community on one of the world's most > > > > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > > > > _______________________________________________ > > > > enlightenment-devel mailing list > > > > [email protected] > > > > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > > > > > > ------------------------------------------------------------ > > > ------------------ > > > Check out the vibrant tech community on one of the world's most > > > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > > > _______________________________________________ > > > enlightenment-devel mailing list > > > [email protected] > > > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > > > > > > > > > > > -- > > Michaël Bouchaud > > ------------------------------------------------------------ > ------------------ > > Check out the vibrant tech community on one of the world's most > > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > > _______________________________________________ > > enlightenment-devel mailing list > > [email protected] > > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > > ------------------------------------------------------------ > ------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > _______________________________________________ > enlightenment-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > -- Michaël Bouchaud ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
