-
Notifications
You must be signed in to change notification settings - Fork 278
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
Changes from 3 commits
c43dd52
46db3f2
130c569
1ed6ba5
6bff15e
c13f80f
8003a4e
49cac99
f93444a
c6a2be5
cb942b4
ec1d136
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,7 +25,7 @@ | |
// THE SOFTWARE. | ||
|
||
using System; | ||
using System.Diagnostics; | ||
using System.Threading.Tasks; | ||
using Cairo; | ||
using Mono.Addins; | ||
using Mono.Addins.Localization; | ||
|
@@ -84,35 +84,12 @@ 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 () | ||
public virtual Task<Gtk.ResponseType> LaunchConfiguration () | ||
{ | ||
if (IsConfigurable) | ||
throw new NotImplementedException ($"{GetType ()} is marked as configurable, but has not implemented LaunchConfiguration"); | ||
} | ||
|
||
/// <summary> | ||
/// Launches the standard configuration dialog for this effect. | ||
/// </summary> | ||
/// <param name="localizer"> | ||
/// 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 e.g. https://github.com/PintaProject/NightVisionEffect/blob/a9ac90a38373cec4c5f69d042998c4ffe35e6438/NightVisionAddin/NightVisionEffect.cs#L53 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah changing the return type to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I think that's the only demo for an effect |
||
{ | ||
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 Task.FromResult (Gtk.ResponseType.Ok); // Placeholder | ||
} | ||
|
||
#region Overridable Render Methods | ||
|
@@ -188,13 +165,6 @@ public virtual BaseEffect Clone () | |
|
||
return effect; | ||
} | ||
|
||
public class ConfigDialogResponseEventArgs : EventArgs | ||
{ | ||
public ConfigDialogResponseEventArgs (bool accepted) { Accepted = accepted; } | ||
|
||
public bool Accepted { get; } | ||
} | ||
} | ||
|
||
/// <summary> | ||
|
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 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 theConfigDialogResponseEventArgs
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 matterResponseType
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 effectsThere 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.
I think
Task<bool>
is good for now while the API is being defined. I just changed it