GNOME Bugzilla – Bug 675804
Cleanups in EphyLocationEntry and EphyLocationController
Last modified: 2018-03-30 16:29:03 UTC
Use actual properties and bindings instead of cumbersome sync functions and notify handlers.
Created attachment 213797 [details] [review] ephy-location-entry: make parameters actual properties So that they are bindable.
Created attachment 213798 [details] [review] ephy-location-controller: use GBinding to sync properties with the entry
Created attachment 213799 [details] [review] ephy-location-controller: use a GBinding to sync the entry address property This also wipes the changes related to ef289f55, which are not really fixing anything as far as I can tell, and cleans up related code.
Created attachment 213800 [details] [review] ephy-location-controller: add missing default case in set/get_property methods
Review of attachment 213800 [details] [review]: git rebase -i is your friend, merge with the first one no?
Review of attachment 213797 [details] [review]: Looks good, merge with the last one.
Review of attachment 213798 [details] [review]: Awesomesauce.
Review of attachment 213799 [details] [review]: If it's possible to delete the code independently from the change please let's do that; otherwise I'll sit down and double-check this later, I'm in a hurry now.
Review of attachment 213800 [details] [review]: This patch is for ephy-location-controller, the first one is for ephy-location-entry, so they are unrelated. Changing status to 'none' as per comment.
Attachment 213797 [details] pushed as ad00117 - ephy-location-entry: make parameters actual properties Attachment 213798 [details] pushed as db67dc8 - ephy-location-controller: use GBinding to sync properties with the entry Attachment 213800 [details] pushed as c4e887a - ephy-location-controller: add missing default case in set/get_property methods I just pushed the rejected one, since the reason for rejection was confusion with where the patch was to be applied and not that it's bad.
Created attachment 215643 [details] [review] ephy-location-controller: remove blocking of entry updates when focused This was introduced as a fix for bug 488680. I couldn't reproduce this bug at all and the code is overly complicated, furthermore, it's stopping me from using GBinding for the entry address synchronization.
Created attachment 215644 [details] [review] ephy-location-controller: use gbinding to sync the entry address
Review of attachment 215643 [details] [review]: So is it me or this code does not actually do anything? It just seems to carefully control the boolean flag, but it's never actually used for anything. If we agree on this then it seems completely safe to remove it.
Review of attachment 215644 [details] [review]: Looks good assuming we commit the previous patch.
(In reply to comment #13) > Review of attachment 215643 [details] [review]: > > So is it me or this code does not actually do anything? It just seems to > carefully control the boolean flag, but it's never actually used for anything. > If we agree on this then it seems completely safe to remove it. Not really, it actually removes the handler that takes care of synchronizing the address with the entry when the entry is focused, so that this doesn't happen while you're typing a new address, but for the life of me I couldn't reproduce this bug at all...
(In reply to comment #15) > Not really, it actually removes the handler that takes care of synchronizing > the address with the entry when the entry is focused, so that this doesn't > happen while you're typing a new address, but for the life of me I couldn't > reproduce this bug at all... You are right! So I think it would be good to know how exactly the code is preventing this from happening before removing this bit. FWIW my brain tells me this can actually happen in ephy (at least I seem to remember experiencing it), so maybe the answer is "not at all"?
Ping about this? Did you look into it?
Second ping.
Comment on attachment 215643 [details] [review] ephy-location-controller: remove blocking of entry updates when focused Marking as 'needs-work' since this needs at least an explanation in the commit, and to drop it from my queue ;)
This is a mass NEEDINFO of all Epiphany bugs with no activity in the past three years. I'm going to be automatically closing old bugs to help us focus on current problems. If you feel this bug is still relevant with Epiphany 3.26 or newer, then please leave any comment here so that I know not to close this one.
This is a mass-close of old bugs currently in the NEEDINFO state. If you think this bug is still relevant, please leave a comment.