patchFreeciv - Patches: patch #3550, Merge update_rect_at_mouse_pos...

 
 
Show feedback again

patch #3550: Merge update_rect_at_mouse_pos into move_mapcanvas (patch 11/60)

Submitted by:  Marko Lindqvist <cazfi>
Submitted on:  Sat 20 Oct 2012 11:08:41 PM UTC  
 
Category: client-gtk-3.0Priority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Planned Release: 2.4.0, 2.5.0, 2.6.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Sat 15 Jun 2013 05:35:12 AM UTC, SVN revision 22944:

Fixed deprecation warnings from create_line_at_mouse_pos()

Patch by Rafa?\197?\130 Mu?\197?\188y?\197?\130o

See patch #3550

(Browse SVN revision 22944)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sat 15 Jun 2013 05:35:06 AM UTC, SVN revision 22943:

Fixed deprecation warnings from create_line_at_mouse_pos()

Patch by Rafa?\197?\130 Mu?\197?\188y?\197?\130o

See patch #3550

(Browse SVN revision 22943)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sat 15 Jun 2013 05:34:58 AM UTC, SVN revision 22942:

Fixed deprecation warnings from create_line_at_mouse_pos()

Patch by Rafa?\197?\130 Mu?\197?\188y?\197?\130o

See patch #3550

(Browse SVN revision 22942)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Tue 11 Jun 2013 08:40:35 AM UTC, comment #14:

Well, technically - patch #3469 comment #37.

Rafał Mużyło <galtgendo>
Mon 10 Jun 2013 09:32:09 PM UTC, 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...

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 10 Jun 2013 09:05:32 PM UTC, comment #12:

I've missed a thing - what about the second patch (file #16748) ?

Rafał Mużyło <galtgendo>
Sun 02 Jun 2013 08:18:30 PM UTC, SVN revision 22930:

Handle gtk3-client selection rectangle drawing directly from move_mapcanvas()
and scroll_mapcanvas() instead of update_rect_at_mouse_pos().

Patch by Rafa?\197?\130 Mu?\197?\188y?\197?\130o

See patch #3550

(Browse SVN revision 22930)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 02 Jun 2013 08:18:24 PM UTC, SVN revision 22929:

Handle gtk3-client selection rectangle drawing directly from move_mapcanvas()
and scroll_mapcanvas() instead of update_rect_at_mouse_pos().

Patch by Rafa?\197?\130 Mu?\197?\188y?\197?\130o

See patch #3550

(Browse SVN revision 22929)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 02 Jun 2013 08:18:17 PM UTC, SVN revision 22928:

Handle gtk3-client selection rectangle drawing directly from move_mapcanvas()
and scroll_mapcanvas() instead of update_rect_at_mouse_pos().

Patch by Rafa?\197?\130 Mu?\197?\188y?\197?\130o

See patch #3550

(Browse SVN revision 22928)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 30 May 2013 08:44:14 PM UTC, 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.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 12 Nov 2012 12:07:00 PM UTC, comment #7:

Made a minor thinko in file #16747 - corrected now.

(file #16752)

Rafał Mużyło <galtgendo>
Sun 11 Nov 2012 11:35:13 PM UTC, comment #6:

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)

Rafał Mużyło <galtgendo>
Sun 11 Nov 2012 09:10:24 PM UTC, comment #5:

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.

Rafał Mużyło <galtgendo>
Sun 11 Nov 2012 07:22:39 PM UTC, comment #4:

> 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")

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 11 Nov 2012 07:02:27 PM UTC, comment #3:

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.

Rafał Mużyło <galtgendo>
Sun 11 Nov 2012 01:12:39 AM UTC, 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.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 21 Oct 2012 12:13:05 AM UTC, comment #1:

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.

Rafał Mużyło <galtgendo>
Sat 20 Oct 2012 11:08:41 PM UTC, original submission:

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?

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #16752:  0001-modify-selection-rectangle-drawing.patch added by galtgendo (3kB - text/x-patch - corrected minor inconsistency in the first patch)
file #16747:  0001-modify-selection-rectangle-drawing.patch added by galtgendo (3kB - text/x-patch - updated patches - code is duplicated, but seems to work)
file #16748:  0002-remove-deprecation-warnings-from-create_line_at_mous.patch added by galtgendo (2kB - text/x-patch - updated patches - code is duplicated, but seems to work)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by galtgendo (Updated the item)
  • -unavailable- added by galtgendo
  • -unavailable- added by cazfi (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 14 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sat 15 Jun 2013 05:35:23 AM UTCcazfiStatusReady For Test=>Done
      Open/ClosedOpen=>Closed
    Mon 10 Jun 2013 09:32:09 PM UTCcazfiStatusDone=>Ready For Test
      Open/ClosedClosed=>Open
    Sun 02 Jun 2013 08:18:40 PM UTCcazfiStatusReady For Test=>Done
      Open/ClosedOpen=>Closed
    Thu 30 May 2013 08:44:14 PM UTCcazfiStatusNone=>Ready For Test
      Assigned toNone=>cazfi
      Planned Release2.5.0=>2.4.0, 2.5.0, 2.6.0
    Mon 12 Nov 2012 12:07:00 PM UTCgaltgendoAttached File-=>Added 0001-modify-selection-rectangle-drawing.patch, #16752
    Sun 11 Nov 2012 11:35:13 PM UTCgaltgendoAttached File-=>Added 0001-modify-selection-rectangle-drawing.patch, #16747
      Attached File-=>Added 0002-remove-deprecation-warnings-from-create_line_at_mous.patch, #16748
    Sun 11 Nov 2012 01:12:39 AM UTCcazfiPlanned Release=>2.5.0
    Sat 20 Oct 2012 11:41:22 PM UTCgaltgendoCarbon-Copy-=>Added
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup