avalonia revamp review #1
Conversation
There was a problem hiding this comment.
eng/linux/package.shuntouched - still compiles UX.Gtk
Things I noticed:
- Missing console app?
- Missing config migration? We should at the very least support migrating from 0.6.3.0 before we make a release with this (i.e. 0.7 release?)
- Adding localization support looks easy, very nice!
Intentionally not reviewed:
- Benchmarks
- Tests
The original PR (OpenTabletDriver#2772) mentions missing filters and tools. I don't think these are necessary to merge as we're not breaking much there (it is master after all), but is obviously needed before a release.
If nothing else, having this merged onto a branch on the main repo would be a really good idea.
| var name = (await _driverDaemon.GetTabletConfiguration(id)).Name; | ||
| var state = await _driverDaemon.GetTabletState(id); |
There was a problem hiding this comment.
Wouldn't it reduce the overhead if these attributes were included in the initial call? Maybe expand the functionality of GetTablets() or add a new function to include these?
| { | ||
| public class Settings | ||
| { | ||
| public static readonly Version CurrentRevision = Version.Parse("0.7.0.0"); |
There was a problem hiding this comment.
change "0.7.0.0" from magic number to a project reference
There was a problem hiding this comment.
This is intended to not follow current OTD's version. May instead use integers to be more explicit about that intention. This version number will only be bumped when settings format actually changed, or when migration is needed.
Another way is to move the declaration via MSBuild-generated assembly attributes, something like
<AssemblyAttribute Include="OpenTabletDriver.Attributes.SettingsFormatAttribute">
<_Parameter1>0.7.0.0</_Parameter1>
</AssemblyAttribute>or
Directory.Build.targets
<Project>
<Target Name="AddOpenTabletDriverSettingsFormat" BeforeTargets="BeforeCompile">
<!-- Handle OpenTabletDriverSettingsFormat -->
<ItemGroup Condition="'@(OpenTabletDriverSettingsFormat->Count())' > 0">
<AssemblyAttribute Include="OpenTabletDriver.Attributes.SettingsFormatAttribute">
<_Parameter1>%(OpenTabletDriverSettingsFormat.Identity)</_Parameter1>
</AssemblyAttribute>
</ItemGroup>
</Target>
</Project>Directory.Build.props
<ItemGroup>
<OpenTabletDriverSettingsFormat>0.7.0.0</OpenTabletDriverSettingsFormat>
</ItemGroup>There was a problem hiding this comment.
Ah, yeah, a monotonic number definitely makes more sense.
There was a problem hiding this comment.
This file should probably be extracted into the single image that we need from it, since I assume we don't need all the images in this file (file tells me it has 7 icons)
There was a problem hiding this comment.
We're not really using this logo anymore. It's just there when the project was initialized and not removed till now.
There was a problem hiding this comment.
Should probably be squashed in so we don't have the icon in the history at all. It's almost 200KB that would be added to every fresh fetch on the main repo
| if (OperatingSystem.IsWindows()) | ||
| return services.AddWindowsServices(); | ||
| // else if (OperatingSystem.IsLinux()) | ||
| // return services.AddLinuxServices(); |
There was a problem hiding this comment.
just commenting: we can probably set up autostart on linux with the help of shell commands or systemd integration
|
|
||
| shortcut.Save(GetShortcutPath()); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
add else if (autoStart && AutoStart) case to check if the already created shortcut matches current path?
There was a problem hiding this comment.
Will add shortcut target check to AutoStart instead, so launching from a different path will lead to AutoStart being false for that binary.
| Width="Auto" | ||
| SharedSizeGroup="Input_LabelColumn" /> | ||
| <ColumnDefinition Width="*" /> | ||
| <ColumnDefinition Width="6*" /> |
There was a problem hiding this comment.
It takes up 6 times more space than other columns with *. Is used to scale slider.
| { | ||
| // clear the navigation list to workaround an avalonia layout bug | ||
| // unfortunately due to this we can't animate the Daemon item into | ||
| // popping up |
There was a problem hiding this comment.
link issue number to avalonia bug
| return; | ||
|
|
||
| e.Handled = true; | ||
| // is there a need to implement forward? |
There was a problem hiding this comment.
Assuming this comment relates to back/forward buttons on side mouse, I would prefer forward being an option if Avalonia supports it
There was a problem hiding this comment.
All the navigation-related code is homebrew. The forward is a bit harder to get right due to viewmodel invalidations. We can write an issue for this for now, but this feature is not going to be part of initial merge.
| { | ||
| } | ||
|
|
||
| protected override bool EnableTray { get; } = IsVariableSet("OTD_TRAY_ICON"); |
There was a problem hiding this comment.
check if tray still exists in avalonia (should be default on btw) no tray, add to missing features
There was a problem hiding this comment.
double check that this (area converter) still exists
missing area converter, add to missing features
| Developers = new[] { "InfinityGhost" }; | ||
| Designers = new[] { "InfinityGhost" }; | ||
| Documenters = new[] { "InfinityGhost" }; |
There was a problem hiding this comment.
Reimplement these in Build.props for an about tab or about dialog? The actual content can be updated later
Revert this commit once the Console application is ready again
|
Needs InfinityGhost/HIDSharpCore#23 on Linux |
|
Bindings editor should probably open as another type of window - currently it gets tiled with my other windows. "About" page in old UI does what I describe. |
|
Transparency is flipped, at least on Linux (Sway) - it goes white/grey with mouse in frame and "normal" without the mouse. Disabling transparency fixes this. |
|
possible missing integration of file: |
|
Aux buttons do not work on my PTH-851 |
|
Odd crash while right clicking stuff: |
|
setting a binding for an incorrect mode (e.g. artist mode binding in absolute mode), applying, and then switching to the correct mode and applying that does not make the bindings work, but changing any bindings (even unrelated ones) and applying following that does fix it |
|
clicking reset (in the save/discard/reset menu in the bottom-right) is too aggressive, it should have a confirmation box or "hold down to 'fill' the button to reset" or similar misclick mitigation |
|
changing output modes does not enable apply/save/discard buttons |
|
switching output mode changing settings on the new output mode results in discard not behaving correctly, basically:
|
|
disconnecting a client (e.g. closing 1 of multiple UI processes) causes other clients to also disconnect which may cause issues when Console app is fixed |
|
"full area" tablet area shortcut does not center |
|
needs X9VoiD#8 to be able to run |
|
meta: open issue for adding "forward button" support in GUI (resolves a convo in this thread) |
This PR exists to easier facilitate review and to hopefully catch any eventual missing changes not correctly ported from
master