Skip to content
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

Made LaunchConfiguration asynchronous at the language level #1019

Merged
Merged
31 changes: 7 additions & 24 deletions Pinta.Core/Effects/BaseEffect.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
// THE SOFTWARE.

using System;
using System.Diagnostics;
using System.Threading.Tasks;
using Cairo;
using Mono.Addins;
using Mono.Addins.Localization;
Expand Down Expand Up @@ -84,10 +84,13 @@ public abstract class BaseEffect
/// Launches the configuration dialog for this effect/adjustment.
/// If IsConfigurable is true, the ConfigDialogResponse event will be invoked when the user accepts or cancels the dialog.
/// </summary>
public virtual void LaunchConfiguration ()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about making the return type a Task<bool>, or some wrapper around a bool to make the meaning more clear, sort of like the ConfigDialogResponseEventArgs from before?

My main thoughts are

  • Gtk.ResponseType has a lot of values in the enum, so it's less clear which values we care about or are possible to have to deal with. It's really just success / failure that matter
  • Ideally the effect API should be as independent as possible from UI stuff like GTK. Using the ResponseType enum probably doesn't cause anything bad like forcing GTK libraries to be loaded / initialized for the unit tests, but it does mean that any API changes when upgrading to future GTK versions have more of an impact on the effects

Copy link
Contributor Author

@Lehonti Lehonti Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Task<bool> is good for now while the API is being defined. I just changed it

/// <returns>Whether the user's response was positive or negative</returns>
public virtual Task<bool> LaunchConfiguration ()
{
if (IsConfigurable)
throw new NotImplementedException ($"{GetType ()} is marked as configurable, but has not implemented LaunchConfiguration");

return Task.FromResult (true); // Placeholder
}

/// <summary>
Expand All @@ -97,22 +100,9 @@ public virtual void LaunchConfiguration ()
/// The localizer for the effect add-in. This is used to fetch translations for the
/// strings in the dialog.
/// </param>
protected void LaunchSimpleEffectDialog (AddinLocalizer localizer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure this signature (or some equivalent way of doing this) is still needed for addins, since this wrapper translates the Mono.Addins.AddinLocalizer into the IAddinLocalizer interface
Addins can't create an AddinLocalizerWrapper manually either since that's an internal class.

e.g. https://github.com/PintaProject/NightVisionEffect/blob/a9ac90a38373cec4c5f69d042998c4ffe35e6438/NightVisionAddin/NightVisionEffect.cs#L53
and
168115e

Copy link
Contributor Author

@Lehonti Lehonti Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I've re-added it, but the return type is a Task<bool>. Is that fine?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah changing the return type to Task<bool> is fine - all the demo add-ins will need to be updated for any API changes before release

Copy link
Contributor Author

@Lehonti Lehonti Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cameronwhite I know that you are referring to API changes and demos in general (in all the extensible parts of Pinta). With that being said, is there another demo effect besides the night vision one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I think that's the only demo for an effect

protected Task<bool> LaunchSimpleEffectDialog (AddinLocalizer localizer)
{
PintaCore.Chrome.LaunchSimpleEffectDialog (this, new AddinLocalizerWrapper (localizer));
}

/// <summary>
/// Emitted when the configuration dialog is accepted or cancelled by the user.
/// </summary>
public event EventHandler<ConfigDialogResponseEventArgs>? ConfigDialogResponse;

/// <summary>
/// Notify that the configuration dialog was accepted or cancelled by the user.
/// </summary>
public void OnConfigDialogResponse (bool accepted)
{
ConfigDialogResponse?.Invoke (this, new ConfigDialogResponseEventArgs (accepted));
return PintaCore.Chrome.LaunchSimpleEffectDialog (this, new AddinLocalizerWrapper (localizer));
}

#region Overridable Render Methods
Expand Down Expand Up @@ -188,13 +178,6 @@ public virtual BaseEffect Clone ()

return effect;
}

public class ConfigDialogResponseEventArgs : EventArgs
{
public ConfigDialogResponseEventArgs (bool accepted) { Accepted = accepted; }

public bool Accepted { get; }
}
}

/// <summary>
Expand Down
56 changes: 24 additions & 32 deletions Pinta.Core/Managers/ChromeManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,15 @@
// THE SOFTWARE.

using System;
using Gtk;
using System.Threading.Tasks;
using Mono.Addins.Localization;

namespace Pinta.Core;

public interface IChromeService
{
Window MainWindow { get; }
void LaunchSimpleEffectDialog (BaseEffect effect, IAddinLocalizer localizer);
Gtk.Window MainWindow { get; }
Task<bool> LaunchSimpleEffectDialog (BaseEffect effect, IAddinLocalizer localizer);
}

public sealed class ChromeManager : IChromeService
Expand All @@ -43,28 +43,25 @@ public sealed class ChromeManager : IChromeService

// NRT - These are all initialized via the Initialize* functions
// but it would be nice to rewrite it to provably non-null.
public Application Application { get; private set; } = null!;
public Window MainWindow { get; private set; } = null!;
public Widget ImageTabsNotebook { get; private set; } = null!;
public Gtk.Application Application { get; private set; } = null!;
public Gtk.Window MainWindow { get; private set; } = null!;
public Gtk.Widget ImageTabsNotebook { get; private set; } = null!;
private IProgressDialog progress_dialog = null!;
private ErrorDialogHandler error_dialog_handler = null!;
private MessageDialogHandler message_dialog_handler = null!;
private SimpleEffectDialogHandler simple_effect_dialog_handler = null!;

public Box? MainToolBar { get; private set; }
public Box ToolToolBar { get; private set; } = null!;
public Widget ToolBox { get; private set; } = null!;
public Box StatusBar { get; private set; } = null!;
public Gtk.Box? MainToolBar { get; private set; }
public Gtk.Box ToolToolBar { get; private set; } = null!;
public Gtk.Widget ToolBox { get; private set; } = null!;
public Gtk.Box StatusBar { get; private set; } = null!;

public IProgressDialog ProgressDialog => progress_dialog;
public Gio.Menu AdjustmentsMenu { get; private set; } = null!;
public Gio.Menu EffectsMenu { get; private set; } = null!;

public ChromeManager ()
{
}
public ChromeManager () { }

#region Public Properties
public PointI LastCanvasCursorPoint {
get => last_canvas_cursor_point;
set {
Expand All @@ -86,40 +83,38 @@ public bool MainWindowBusy {
MainWindow.Cursor = Gdk.Cursor.NewFromName (Pinta.Resources.StandardCursors.Default, null);
}
}
#endregion

#region Public Methods
public void InitializeApplication (Gtk.Application application)
{
Application = application;
}

public void InitializeWindowShell (Window shell)
public void InitializeWindowShell (Gtk.Window shell)
{
MainWindow = shell;
}

public void InitializeToolToolBar (Box toolToolBar)
public void InitializeToolToolBar (Gtk.Box toolToolBar)
{
ToolToolBar = toolToolBar;
}

public void InitializeMainToolBar (Box mainToolBar)
public void InitializeMainToolBar (Gtk.Box mainToolBar)
{
MainToolBar = mainToolBar;
}

public void InitializeStatusBar (Box statusbar)
public void InitializeStatusBar (Gtk.Box statusbar)
{
StatusBar = statusbar;
}

public void InitializeToolBox (Widget toolbox)
public void InitializeToolBox (Gtk.Widget toolbox)
{
ToolBox = toolbox;
}

public void InitializeImageTabsNotebook (Widget notebook)
public void InitializeImageTabsNotebook (Gtk.Widget notebook)
{
ImageTabsNotebook = notebook;
}
Expand Down Expand Up @@ -150,12 +145,12 @@ public void InitializeSimpleEffectDialog (SimpleEffectDialogHandler handler)
simple_effect_dialog_handler = handler;
}

public void ShowErrorDialog (Window parent, string message, string body, string details)
public void ShowErrorDialog (Gtk.Window parent, string message, string body, string details)
{
error_dialog_handler (parent, message, body, details);
}

public void ShowMessageDialog (Window parent, string message, string body)
public void ShowMessageDialog (Gtk.Window parent, string message, string body)
{
message_dialog_handler (parent, message, body);
}
Expand All @@ -165,11 +160,10 @@ public void SetStatusBarText (string text)
OnStatusBarTextChanged (text);
}

public void LaunchSimpleEffectDialog (BaseEffect effect, IAddinLocalizer localizer)
public Task<bool> LaunchSimpleEffectDialog (BaseEffect effect, IAddinLocalizer localizer)
{
simple_effect_dialog_handler (effect, localizer);
return simple_effect_dialog_handler (effect, localizer);
}
#endregion

private void OnLastCanvasCursorPointChanged ()
{
Expand All @@ -181,10 +175,8 @@ private void OnStatusBarTextChanged (string text)
StatusBarTextChanged?.Invoke (this, new TextChangedEventArgs (text));
}

#region Public Events
public event EventHandler? LastCanvasCursorPointChanged;
public event EventHandler<TextChangedEventArgs>? StatusBarTextChanged;
#endregion
}

public interface IProgressDialog
Expand All @@ -197,6 +189,6 @@ public interface IProgressDialog
event EventHandler<EventArgs> Canceled;
}

public delegate void ErrorDialogHandler (Window parent, string message, string body, string details);
public delegate void MessageDialogHandler (Window parent, string message, string body);
public delegate void SimpleEffectDialogHandler (BaseEffect effect, IAddinLocalizer localizer);
public delegate void ErrorDialogHandler (Gtk.Window parent, string message, string body, string details);
public delegate void MessageDialogHandler (Gtk.Window parent, string message, string body);
public delegate Task<bool> SimpleEffectDialogHandler (BaseEffect effect, IAddinLocalizer localizer);
32 changes: 11 additions & 21 deletions Pinta.Core/Managers/LivePreviewManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ internal LivePreviewManager (
public event EventHandler<LivePreviewRenderUpdatedEventArgs>? RenderUpdated;
public event EventHandler<LivePreviewEndedEventArgs>? Ended;

public void Start (BaseEffect effect)
public async void Start (BaseEffect effect)
{
if (live_preview_enabled)
throw new InvalidOperationException ("LivePreviewManager.Start() called while live preview is already enabled.");
Expand Down Expand Up @@ -137,23 +137,13 @@ public void Start (BaseEffect effect)
renderer.Start (effect, layer.Surface, live_preview_surface);

if (effect.IsConfigurable) {
EventHandler<BaseEffect.ConfigDialogResponseEventArgs>? handler = null;
handler = (_, args) => {
if (!args.Accepted) {
chrome_manager.MainWindowBusy = true;
Cancel ();
} else {
chrome_manager.MainWindowBusy = true;
Apply ();
}

// Unsubscribe once we're done.
effect.ConfigDialogResponse -= handler;
};

effect.ConfigDialogResponse += handler;

effect.LaunchConfiguration ();
bool response = await effect.LaunchConfiguration ();
chrome_manager.MainWindowBusy = true;
if (response)
Apply ();
else
Cancel ();

} else {
chrome_manager.MainWindowBusy = true;
Expand Down Expand Up @@ -277,23 +267,23 @@ void EffectData_PropertyChanged (object? sender, PropertyChangedEventArgs e)
private sealed class Renderer : AsyncEffectRenderer
{
readonly LivePreviewManager manager;
readonly ChromeManager chrome_manager;
readonly ChromeManager chrome;
internal Renderer (
LivePreviewManager manager,
AsyncEffectRenderer.Settings settings,
ChromeManager chromeManager)
ChromeManager chrome)
: base (settings)
{
this.manager = manager;
this.chrome_manager = chromeManager;
this.chrome = chrome;
}

protected override void OnUpdate (
double progress,
RectangleI updatedBounds)
{
Debug.WriteLine (DateTime.Now.ToString ("HH:mm:ss:ffff") + " LivePreviewManager.OnUpdate() progress: " + progress);
chrome_manager.ProgressDialog.Progress = progress;
chrome.ProgressDialog.Progress = progress;
manager.FireLivePreviewRenderUpdatedEvent (progress, updatedBounds);
}

Expand Down
3 changes: 2 additions & 1 deletion Pinta.Effects/Adjustments/BrightnessContrastEffect.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
/////////////////////////////////////////////////////////////////////////////////

using System;
using System.Threading.Tasks;
using Cairo;
using Pinta.Core;
using Pinta.Gui.Widgets;
Expand Down Expand Up @@ -50,7 +51,7 @@ void HandleEffectDataPropertyChanged (object? sender, System.ComponentModel.Prop
table_calculated = false;
}

public override void LaunchConfiguration ()
public override Task<bool> LaunchConfiguration ()
=> chrome.LaunchSimpleEffectDialog (this);

public override void Render (ImageSurface src, ImageSurface dest, ReadOnlySpan<RectangleI> rois)
Expand Down
9 changes: 7 additions & 2 deletions Pinta.Effects/Adjustments/CurvesEffect.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Threading.Tasks;
using Cairo;
using Pinta.Core;

Expand Down Expand Up @@ -40,19 +41,23 @@ public CurvesEffect (IServiceProvider services)
EffectData = new CurvesData ();
}

public override void LaunchConfiguration ()
public override Task<bool> LaunchConfiguration ()
{
TaskCompletionSource<bool> completionSource = new ();

CurvesDialog dialog = new (chrome, Data) {
Title = Name,
IconName = Icon,
};

dialog.OnResponse += (_, args) => {
OnConfigDialogResponse (args.ResponseId == (int) Gtk.ResponseType.Ok);
completionSource.SetResult (Gtk.ResponseType.Ok == (Gtk.ResponseType) args.ResponseId);
dialog.Destroy ();
};

dialog.Present ();

return completionSource.Task;
}

public override void Render (ImageSurface src, ImageSurface dest, ReadOnlySpan<RectangleI> rois)
Expand Down
3 changes: 2 additions & 1 deletion Pinta.Effects/Adjustments/HueSaturationEffect.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
/////////////////////////////////////////////////////////////////////////////////

using System;
using System.Threading.Tasks;
using Cairo;
using Pinta.Core;
using Pinta.Gui.Widgets;
Expand All @@ -34,7 +35,7 @@ public HueSaturationEffect (IServiceProvider services)
EffectData = new HueSaturationData ();
}

public override void LaunchConfiguration ()
public override Task<bool> LaunchConfiguration ()
=> chrome.LaunchSimpleEffectDialog (this);

private UnaryPixelOp CreateOptimalOp ()
Expand Down
13 changes: 7 additions & 6 deletions Pinta.Effects/Adjustments/LevelsEffect.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
/////////////////////////////////////////////////////////////////////////////////

using System;
using System.Threading.Tasks;
using Cairo;
using Pinta.Core;

Expand Down Expand Up @@ -40,23 +41,23 @@ public LevelsEffect (IServiceProvider services)
EffectData = new LevelsData ();
}

public override void LaunchConfiguration ()
public override Task<bool> LaunchConfiguration ()
{
TaskCompletionSource<bool> completionSource = new ();

LevelsDialog dialog = new (chrome, workspace, Data) {
Title = Name,
IconName = Icon,
};

dialog.OnResponse += (_, args) => {

if (args.ResponseId == (int) Gtk.ResponseType.None)
return;

OnConfigDialogResponse (args.ResponseId == (int) Gtk.ResponseType.Ok);
completionSource.SetResult (Gtk.ResponseType.Ok == (Gtk.ResponseType) args.ResponseId);
dialog.Destroy ();
};

dialog.Present ();

return completionSource.Task;
}

public override void Render (ImageSurface src, ImageSurface dest, ReadOnlySpan<RectangleI> rois)
Expand Down
Loading
Loading