Changes between Initial Version and Version 1 of Ticket #7888, comment 19


Ignore:
Timestamp:
2012-08-04T22:11:53+02:00 (13 years ago)
Author:
bastiK

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #7888, comment 19

    initial v1  
    11Replying to [comment:16 akks]:
    22> I am trying to understand all SelectAction code, especially modificator-related on my Windows.
    3 >
     3
     4Some parts of the JOSM code I try not to look at for too long, because it makes my eyes bleed. SelectAction is one of them. But you are trying to improve the situation, so I'll try to help the best I can.
     5
    46> I have some questions, can someone who wrote the code (or know it well) help?
    57>
    … …  
    2123>
    2224> What is the logic and purpose of such strange action?
    23 >
     25
     26I think it is due to the history of the code: The alt cycle behaviour was added by Christian Müller in Sep.-Oct. 2010. Scaling was implemented 2 Months later ([3702]) by Olivier Croquette. Probably the second author didn't bother to rework that part of the code.
     27
    2428> * We have two "dangerous" lines in code that change modificator flags:
    2529> {{{
    2630> #!java
    2731> alt = alt || Main.pref.getBoolean("selectaction.cycles.multiple.matches", false);
    28 > ...
     32> }}}
     33
     34Christian suggested, that "false" should be replaced by "true" after a testing period, but it did not happen so far.
     35
     36> {{{
     37> #!java
    2938>  shift |= getCurrentDataSet().getSelected().containsAll(prims);
    3039> }}}
    31 > http://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/actions/mapmode/SelectAction.java#L845
    32 > http://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/actions/mapmode/SelectAction.java#L774
    33 >
    34 > (after next call of updateKeyModfiers '''alt''' and '''shift''' will be rewritten anyway, but before that happen, changed variable is used)
    35 > Is there any purpose or we can just add some temporary variable instead of alt?
    36 >
     40
     41I would try to understand the purpose of this line and change it to a local variable unless there's a real semantic difference.
     42