GNOME Bugzilla – Bug 740414
Starting via ActivateAction on 'show-location' does not work.
Last modified: 2015-03-18 20:46:36 UTC
Hi, When you start weather via ActivateAction, passing a location as parameter we will add the location to the model. But the currentLocationChanged method on the model will always get called and adds another location. So the location we sent will not get shown. What is the best way of solving this. The patch I will attach does not feel all that nice. Thanks! Jonas
Created attachment 291073 [details] [review] main: Delay add for 'show-location' action When Weather is started with ActivateAction on the 'show-location' action we add the given location to the model. However the CurrentLocationControler will override this action by running the currentLocationChanged method on the model. This means that we will not show the location given to 'show-location'. This does not really feel like the correct way of solving this.
The patch will break the case when you do activateaction on an already open window, since then we are not guaranteed a change of current location. I just wanted to show that the patch attached "solves" this for me. I don't know the code base enough to clearly see the Right Way to solve this. Any pointers?
Sorry for taking so long to review this. I believe the right way to do this is just to flag the window in a way that CurrentLocationController will not change the current location and will simply append the new location to the model (which will then update the popover)
Created attachment 297871 [details] [review] src/Makefile.am: Use -f when linking gnome-weather Without force the install will complain if 'gnome-weather' is already there.
Created attachment 297872 [details] [review] Only show current location if we are on the search page Make sure we do not display the current location when another location currently is being shown.
Created attachment 297918 [details] [review] Only show location updates if we are on the search page Make sure we do not display the updated location when another location currently is being shown.
There is an other issue, maybe you are aware of it? Since all window share the same model, all windows will react to the show-info signal and show the location. So if you have two window up and do a show-location action from Maps, all window will show that location. And even if you discard the show-location action... If you have three gnome-weather windows and use the popover to select a location, all windows will show it. Any ideas how to address this?
Review of attachment 297871 [details] [review]: Sure
Review of attachment 297918 [details] [review]: 1) I'd like to stick with isCurrentLocation as name of the parameter 2) If the current location is the one being shown, then we should switch to the new location - the isCurrentLocation=true location is a sort of placeholder for wherever geoclue tells us we are
(In reply to Jonas Danielsson from comment #7) > There is an other issue, maybe you are aware of it? > > Since all window share the same model, all windows will react to the > show-info signal and show the location. > > So if you have two window up and do a show-location action from Maps, all > window will show that location. > > And even if you discard the show-location action... If you have three > gnome-weather windows and use the popover to select a location, all windows > will show it. > > Any ideas how to address this? Right, that's a bug. In practice, the show-info signal should go away from the model, and turn into a method call on the window. We can achieve that by calling it after loading the model, and hooking up the window with the currentlocationcontroller. Then we would move some of the logic in window. Given that you're already working on this, do you want to continue? Or would you prefer me to finish?
Review of attachment 297918 [details] [review]: 1) Ok! 2) I don't quite follow. Do you mean that if what is being shown at the moment is what we currently have as "current location"? Then replace it with what geoclue feels is a new "current location" ? So show a info marked as isCurrentLocation if a) we are on the search page or b) we are showing something that previously was the current location?
(In reply to Jonas Danielsson from comment #11) > Review of attachment 297918 [details] [review] [review]: > > 1) Ok! > > 2) I don't quite follow. Do you mean that if what is being shown at the > moment is what we currently have as "current location"? Then replace it with > what geoclue feels is a new "current location" ? So show a info marked as > isCurrentLocation if a) we are on the search page or b) we are showing > something that previously was the current location? Geoclue locations are marked by info._isCurrentLocation. But yes, the logic is "show the new geoclue location" if a) we're showing nothing (search page) b) we're showing the previous geoclue location
Created attachment 298187 [details] [review] Refactor adding and displaying of locations Remove the 'show-info' signal and all display related methods from the shared world model. And have explicit calls to the showInfo method on the window object used instead. This fixes a bug where adding a location to the shared model would show the new location on all open windows. Also make sure to only display a new current location when we are not displaying another location, or when we are displaying the previous current location.
So, this patch is a suggestion on how to handle this. Sorry I didn't have a lot of time to work on it, so a review and sanity check would be nice. I also wonder if the app/world.js modul should sync its listbox with the world model at start and not just listen to add/remove? Seems it does not get populated in a new window?
Comment on attachment 297871 [details] [review] src/Makefile.am: Use -f when linking gnome-weather Attachment 297871 [details] pushed as 2f3544a - src/Makefile.am: Use -f when linking gnome-weather
Created attachment 298206 [details] [review] app/world.js: Check if model is already populated at init If this is not the first window, the model might already been loaded. Make sure we add all locations currently in the model.
Created attachment 298207 [details] [review] Refactor adding and displaying of locations Remove the 'show-info' signal and all display related methods from the shared world model. And have explicit calls to the showInfo method on the window object used instead. This fixes a bug where adding a location to the shared model would show the new location on all open windows. Also make sure to only display a new current location when we are not displaying another location, or when we are displaying the previous current location.
Created attachment 298211 [details] [review] app/world.js: Check if model is already populated at init If this is not the first window, the model might already been loaded. Make sure we add all locations currently in the model.
Created attachment 298212 [details] [review] Refactor adding and displaying of locations Remove the 'show-info' signal and all display related methods from the shared world model. And have explicit calls to the showInfo method on the window object used instead. This fixes a bug where adding a location to the shared model would show the new location on all open windows. Also make sure to only display a new current location when we are not displaying another location, or when we are displaying the previous current location.
Created attachment 298346 [details] [review] app/world.js: Check if model is already populated at init If this is not the first window, the model might already been loaded. Make sure we add all locations currently in the model.
Created attachment 298347 [details] [review] Refactor adding and displaying of locations Remove the 'show-info' signal and all display related methods from the shared world model. And have explicit calls to the showInfo method on the window object used instead. This fixes a bug where adding a location to the shared model would show the new location on all open windows. Also make sure to only display a new current location when we are not displaying another location, or when we are displaying the previous current location.
Review of attachment 298346 [details] [review]: Looks good.
Review of attachment 298347 [details] [review]: Much better, thank you!
I pushed this so that it could make it into today release. Thanks again for the patches!
*** Bug 745453 has been marked as a duplicate of this bug. ***
*** Bug 744242 has been marked as a duplicate of this bug. ***