[Refactor] Move autoscaling methods in AutoScaling class #2709
Conversation
ShahzaibIbrahim
force-pushed
the
master-399
branch
from
2e7eb5f to
b6f92bf
Compare
ShahzaibIbrahim
force-pushed
the
master-399
branch
from
b6f92bf to
0c71467
Compare
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/AutoScaling.java
Show resolved
Hide resolved
| /** | ||
| * Utility for checking and enabling monitor-specific auto scaling. | ||
| * @since 3.132 | ||
| */ |
There was a problem hiding this comment.
So you want this to be exported as API? If that's the case, make sure that every visible method in it is properly named and documented.
ShahzaibIbrahim
force-pushed
the
master-399
branch
from
0c71467 to
347ddc3
Compare
ShahzaibIbrahim
force-pushed
the
master-399
branch
from
347ddc3 to
0bcd2d2
Compare
A new utility solely responsible for taking care of autoscaling methods. Also introduced new system property swt.autoScale.force that can be set to true in order to disable the compatibility error for setting the autoscale value when update on runtime is enabled.
ShahzaibIbrahim
force-pushed
the
master-399
branch
from
0bcd2d2 to
31f1b58
Compare
There was a problem hiding this comment.
I would be in favor of doing such a change more incrementally. Obviously, there is code in Platform UI depending on (internal) methods of DPIUtil. Moving them to another class means that Platform UI code becomes incompatible once we merge the SWT PR and if we merge the Platform UI PR at the same time (without knowing if it works as the CI will fail) will make Platform UI builds fail until the next I-Build.
Thus, in such a case a multi-step transition should be made by refactoring and maybe adding methods while still keeping the old ones (as delegates), then adapt the consumers (such as Platform UI) and once that is done remove the delegates that became obsolete.
It also seems like the AutoScaling class and DPIUtil would be highly mutually dependent. This could be an indicator that splitting them up is not a good idea. In any case, AutoScaling seems like it disposes much new public API that we will tie ourselves to. I know that I proposed to make some its functionality API, but seeing it I am not so sure anymore whether we should really do it or better keep it internal (like with DPIUtil) and access the internal API from Platform UI for now. At least, whatever we make public should be as lean as possible and in my opinion we should definitely not make something like an autoscale mode publicly accessible.
| static { | ||
| autoScaleValue = System.getProperty (SWT_AUTOSCALE); | ||
| } | ||
| private static final Set<String> ALLOWED_VALUES = Set.of("false", "quarter", "exact"); |
There was a problem hiding this comment.
In my opinion, we should even remove the "false" option. And the variable should have a more specific name, as is's unclear for what these values are allowed.
| private static final Set<String> ALLOWED_VALUES = Set.of("false", "quarter", "exact"); | |
| private static final Set<String> ALLOWED_AUTOSCALE_VALUES_FOR_UPDATE_ON_RUNTIME = Set.of("quarter", "exact"); |
3 participants
A new utility solely responsible for taking care of autoscaling methods. Also introduced new system property swt.autoScale.force that can be set to true in order to disable the compatibility error for setting the autoscale value when update on runtime is enabled.