-
-
Notifications
You must be signed in to change notification settings - Fork 13
refactor: clean architecture #260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…ling Switched the log viewer from a polling mechanism to an observable pattern. The previous approach's constant 50ms checks were inefficient and consumed unnecessary CPU cycles. This change eliminates that fixed delay and constant polling overhead, allowing new log entries to appear instantly as they're generated.
Color still needs a string converter.
|
|
||
| public partial class SettingsHomePageView : UserControl | ||
| { | ||
| private SettingsHomePageViewVM ViewModel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An opening brace should not be followed by a blank line.
| private SettingsHomePageViewVM ViewModel; |
| return process.ExitCode; | ||
| } | ||
|
|
||
| return -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A closing brace should not be preceded by a blank line.
| return -1; |
|
|
||
| public void Emit(LogEvent logEvent) | ||
| { | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An opening brace should not be followed by a blank line.
| ICaptureService CaptureService) | ||
| : CaptureBase(MainWindowService, NotificationService, UploadManager, DelayService, LoggerService, CaptureService) | ||
| { | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An opening brace should not be followed by a blank line.
| if (destinationType != typeof(string)) return base.ConvertTo(context, culture, value, destinationType) ?? throw new FormatException($"Cannot convert '{value}' to ImageSharp Color."); | ||
| var list = (List<string>)value; | ||
| return string.Join(", ", list); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A closing brace should not be preceded by a blank line.
| Directory.Delete(testDir, recursive: true); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A closing brace should not be preceded by a blank line.
…hitecture # Conflicts: # SnapX.Avalonia/ViewModels/HomePageViewModel.cs
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
It's not needed to remove anymore, the 6.2GiB from the Haskell runtime removal should be all that's needed?
|
|
||
| return await responseMessage.Content.ReadAsStringAsync(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A closing brace should not be preceded by a blank line.
| return await responseMessage.Content.ReadAsStringAsync(); |
…hitecture # Conflicts: # SnapX.Core/SnapX.Core.csproj
…hitecture # Conflicts: # README.md # SnapX.Core/SnapX.cs
…ask updates - Now scales to a large amount of items without flickering. - Resolved missing symbol 'FindIndex' by introducing an extension method for AvaloniaList<T>. - Optimized recentTasks update logic: - Replaced full list clear/add with incremental mutation using RemoveAt and Insert. - Used reverse iteration for safe in-place removal of old items (`toRemove`). - Applied targeted replacement for items in `toUpdate` via FindIndex. - Prepended new items (`toAdd`) to maintain order and avoid unnecessary sorting. - Wrapped UI update logic in Dispatcher.UIThread.InvokeAsync to ensure thread safety. - Appended GetTask().ConfigureAwait(false) to ensure the continuation is offloaded from the UI thread, improving responsiveness and avoiding unnecessary context switches. These changes improve UI smoothness, avoid layout flicker, prevent unnecessary reallocation, and ensure the async flow is context-aware and lightweight.
| List<int> toRemove; | ||
|
|
||
| newDesiredTasks = tasks | ||
| .OrderByDescending(item => item.task.Id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opening braces should not be preceded by blank line.
| .OrderByDescending(item => item.task.Id) |
| // Warning: Computations on the UIThread are precious. | ||
| await Dispatcher.UIThread.InvokeAsync(() => | ||
| { | ||
| if (newDesiredTasks.Count > 50_000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An opening brace should not be followed by a blank line.
| if (newDesiredTasks.Count > 50_000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it mean? There is no line?
| TypeInfoResolver = SettingsContext.Default | ||
| TypeInfoResolver = SettingsContext.Default, | ||
| Converters = { new JsonStringEnumConverter() } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A closing brace should not be preceded by a blank line.
|
Why does this always send reviews like this every big PR?
|
That's because the CodeFactor bot is meant to improve code quality, but unfortunately, all it's done is add noise. |
…hitecture # Conflicts: # SnapX.CLI/Program.cs # SnapX.Core/SnapX.Core.csproj
…hitecture # Conflicts: # SnapX.Avalonia/Views/HomePageView.axaml # SnapX.Avalonia/Views/HomePageView.axaml.cs
|
Just why.
|
I understand the reaction 😅. As someone curious & technologically adventurous, I’m always exploring ways to streamline workflows, and this was an experiment to see if auto-generating PR overviews could save time.
|
|
This PR is being marked as stale due to inactivity. |
|
Nuhuh. |
|
💀 What an insane reply.
|
Preface
SnapX is our cross-platform evolution of ShareX. While we’ve already removed the original WinForms UI code—a necessary step for making SnapX build and run beyond Windows—this left significant gaps in the core logic.
Much of SnapX.Core was historically intertwined with direct UI calls. Removing the WinForms layer exposed how deeply this coupling ran, leaving several core features incomplete or blocked because the business logic still assumed UI components were directly accessible.
Features like region-based screen OCR or triggering notifications are key examples. Previously, these relied on tightly integrated UI calls, which no longer exist. As a result, SnapX.Core couldn’t properly request UI-driven actions or surface user interactions.
This PR addresses that gap to ensure SnapX can move forward toward a stable cross-platform release.
Description of Change
This PR refactors SnapX.Core to decouple it cleanly from any UI dependencies. Instead of assuming direct calls to UI elements, the core now exposes well-defined interfaces and contracts for operations that require user interaction or visual feedback.
Key changes include:
This ensures that SnapX.Core can remain UI-agnostic while enabling frontends like Avalonia to implement platform-specific user interactions.
Possible Alternatives
We briefly considered patching core logic with conditionals to call Avalonia-specific methods directly. However, that approach would have reintroduced tight coupling between SnapX.Core and SnapX.Avalonia, defeating the purpose of modularizing the app for cross-platform support.
Another alternative was to postpone this refactoring and only address UI calls feature by feature. We decided against this, as it would have prolonged technical debt and slowed down the release schedule for upcoming versions.
Implementation Details
Notes
This refactor paves the way for completing several previously blocked features, including region-based OCR and proper notification handling.
It’s expected to land as part of the upcoming bi-weekly v0.4.0 pre-release.
Additionally, I've fixed a lot of the types for the ApplicationConfig, that the unfinished types caused corruption.
Future work includes:
Disclosure: This PR overview was auto-generated by ChatGPT. As an experiment to save developer time.