[Freeciv-Dev] [patch #3550] Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60)
Update of patch #3550 (project freeciv): Status: Ready For Test = Done Open/Closed:Open = Closed ___ Reply to this item at: http://gna.org/patch/?3550 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3550] Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60)
Follow-up Comment #14, patch #3550 (project freeciv): Well, technically - patch #3469 comment #37. ___ Reply to this item at: http://gna.org/patch/?3550 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3550] Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60)
Follow-up Comment #12, patch #3550 (project freeciv): I've missed a thing - what about the second patch (file #16748) ? ___ Reply to this item at: http://gna.org/patch/?3550 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3550] Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60)
Update of patch #3550 (project freeciv): Status:Done = Ready For Test Open/Closed: Closed = Open ___ Follow-up Comment #13: I've missed a thing - what about the second patch (file #16748) ? This is the reason separate patches should go to separate tickets... ___ Reply to this item at: http://gna.org/patch/?3550 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3550] Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60)
Update of patch #3550 (project freeciv): Status: Ready For Test = Done Open/Closed:Open = Closed ___ Reply to this item at: http://gna.org/patch/?3550 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3550] Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60)
Update of patch #3550 (project freeciv): Status:None = Ready For Test Assigned to:None = cazfi Planned Release: 2.5.0 = 2.4.0, 2.5.0, 2.6.0 ___ Follow-up Comment #8: Ok, it seems that the helper function that both would call I wanted introduced actually exist already named update_selection_rectangle(), and your patch already uses it. ___ Reply to this item at: http://gna.org/patch/?3550 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3550] Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60)
Follow-up Comment #7, patch #3550 (project freeciv): Made a minor thinko in file #16747 - corrected now. (file #16752) ___ Additional Item Attachment: File name: 0001-modify-selection-rectangle-drawing.patch Size:2 KB ___ Reply to this item at: http://gna.org/patch/?3550 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3550] Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60)
Follow-up Comment #3, patch #3550 (project freeciv):
In case of update_rect_at_mouse_pos, the problem comes down to a simple thing
- the function signature is 'void update_rect_at_mouse_pos(*void*)'.
The already stated reason comes into play, the same one I was against
gdk_display_get_device_manager 'solution' in the patch in bug #20097 - the
event *knows* exactly where mouse is when the event happened,
gdk_display_get_device_manager says only where the mouse is while the
*function* runs.
While usually those positions (due to human reaction speed vs cpu power) are
very close, that still means the code would be wrong.
So, with such signature, there's no reasonable way to pass the coordinates
from the event handler to this function.
create_line_at_mouse_pos is a different case: as I said in the comment of that
patch, the only reasonable usecase is already handled elsewhere in the code,
that is the lines are effectively drawn twice now - once by
create_line_at_mouse_pos, the second time by move_{map,overview}canvas -
therefore non-empty create_line_at_mouse_pos is redundant...well, at least
AFAICT.
Well, perhaps cur_x/cur_y could be used here instead of trying to query the
pointer, but all those global vars make me cringe.
I need to think about it.
On a surprisingly related note: that cpu usage jump when a *selected* unit is
on screen (due to that animated circle) is quite annoying.
___
Reply to this item at:
http://gna.org/patch/?3550
___
Message sent via/by Gna!
http://gna.org/
___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3550] Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60)
Follow-up Comment #4, patch #3550 (project freeciv): the problem comes down to a simple thing - the function signature is 'void update_rect_at_mouse_pos(void)'. That's why I proposed new helper function with proper signature (taking coordinates as parameters) ___ Reply to this item at: http://gna.org/patch/?3550 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3550] Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60)
Follow-up Comment #5, patch #3550 (project freeciv): Well, it seems we don't quite understand each other, but OK. I went with the extreme duplication instead. You'll see what I mean once I'll attach updated version of both patches. ___ Reply to this item at: http://gna.org/patch/?3550 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3550] Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60)
Follow-up Comment #6, patch #3550 (project freeciv): As noted, I've decided if there's no good way to do it, I'll stick with a model that sort of works. I've still merged the active content of update_rect_at_mouse_pos into the calbacks that used it, but updated the old code in regard of deprecation warnings. That meant touching up the code in gui_main.c, so that it works in the same way mapctrl.c does. (file #16747, file #16748) ___ Additional Item Attachment: File name: 0001-modify-selection-rectangle-drawing.patch Size:2 KB File name: 0002-remove-deprecation-warnings-from-create_line_at_mous.patch Size:2 KB ___ Reply to this item at: http://gna.org/patch/?3550 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3550] Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60)
Update of patch #3550 (project freeciv): Planned Release: = 2.5.0 ___ Follow-up Comment #2: I see. How about refactoring this to make separate function taking coordinates as parameters that both move_mapcanvas() and update_rect_at_mouse_pos() call, so that API too would work as expected. All this probably applies to patch (12/60): hollow out create_line_at_mouse_pos too. ___ Reply to this item at: http://gna.org/patch/?3550 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3550] Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60)
URL: http://gna.org/patch/?3550 Summary: Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60) Project: Freeciv Submitted by: cazfi Submitted on: Sun 21 Oct 2012 02:08:41 AM EEST Category: client-gtk-3.0 Priority: 5 - Normal Status: None Privacy: Public Assigned to: None Originator Email: Open/Closed: Open Discussion Lock: Any Planned Release: ___ Details: This ticket is about handling patch 11/60 (merge update_rect_at_mouse_pos into move_mapcanvas) from Rafał Mużyło's patchset in patch #3469 The patch inlines contents of update_rect_at_mouse_pos() to move_mapcanvas() leaving former function empty. This results in simpler implementation. I have not investigated issue very deeply, but update_rect_at_mouse_pos() is part of gui API towards client common code, and it is in fact called from there too. Leaving that function empty, i.e., it not doing what API promises, seems wrong. Or can wew be sure that in case of gtk3 its (API defined) implementation would be always redundant? ___ Reply to this item at: http://gna.org/patch/?3550 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3550] Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60)
Follow-up Comment #1, patch #3550 (project freeciv): Well, TBH, there's one major catch here: while things work as-is both in gtk2 and gtk3 clients, technically they only do that due to luck. Even back in gtk2 its devs stressed that *all* of the drawing should be done in 'expose-event' handler. It's still the same for gtk3 and 'draw'. The way update_rect_at_mouse_pos() works (well, more exactly update_selection_rectangle(x, y)), regardless of whether it's merged or not, already pushes it. But I don't see any way of implementing this without a major rewrite of not only the client, but probably common code too. I've merged it, cause while due to human reaction time, doing it like city_activated_callback (cityrep.c) and impr_callback (citydlg.c) do (which I didn't touched and already explained why it's IMHO unfixably broken) would *seem* to work, it's the most natural place with access to the required data, but at the same time, that data can't be as reliably retrieved anywhere else. ___ Reply to this item at: http://gna.org/patch/?3550 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
