Added arguments to export layout to image (#365)

[Atria1234]

AD can now be started with 3 different verbs:

• askAdmin
• open (opens specified file)
• export (exports specified file into specified image, allows specifying export parameters)

[StingMcRay]

maybe odd Q : what does askAdmin ?

[Atria1234]

It is an old behaviour. Something to do about inability to update AD files during update to new version of AD.

Program should restart while asking user for admin access. And this verb was there to prevent infinite loop of restarting when for some reason even admin access isn't enough to update AD files.

[FroggieFrog]

If you have time it would be great if you could add a document (in the doc folder) to describe the possible values and the syntax.

[FroggieFrog]

I managed to make the console output work as I would expect it. But I don't know how to add diff-files in a comment.
So please copy/replace it to your source code and test it out. Maybe you have different expectations.

App.xaml.cs

// at the start of Application_Startup(...)
try
{
    StartupArguments = ArgumentParser.Parse(e.Args);
}
catch (HelpException ex)
{
    ConsoleManager.Show();
    Console.WriteLine(ex.HelpText);
    if (ConsoleManager.StartedWithoutConsole)
    {
        Console.WriteLine("Press enter to exit");
        Console.ReadLine();
    }
    ConsoleManager.Hide();
    Environment.Exit(0);
}

ConsoleManager.cs

using System;
using System.Runtime.InteropServices;
using System.Security;

namespace AnnoDesigner
{
    /// <summary>
    /// Console manager class for showing console when help text should be shown.
    /// </summary>
    /// <remarks>
    /// Source: https://stackoverflow.com/a/718505
    /// </remarks>
    [SuppressUnmanagedCodeSecurity]
    public static class ConsoleManager
    {
        private const string Kernel32_DllName = "kernel32.dll";

        [DllImport(Kernel32_DllName)]
        private static extern bool AllocConsole();

        [DllImport(Kernel32_DllName)]
        private static extern bool FreeConsole();

        [DllImport(Kernel32_DllName)]
        private static extern IntPtr GetConsoleWindow();

        [DllImport(Kernel32_DllName)]
        private static extern bool AttachConsole(int dwProcessId);
        private const int ATTACH_PARENT_PROCESS = -1;

        public static bool HasConsole
        {
            get { return GetConsoleWindow() != IntPtr.Zero; }
        }

        public static bool StartedWithoutConsole { get; private set; }

        /// <summary>
        /// Creates a new console instance if the process is not attached to a console already.
        /// </summary>
        public static void Show()
        {
            if (!AttachConsole(ATTACH_PARENT_PROCESS))
            {
                AllocConsole();

                StartedWithoutConsole = true;
            }
            else
            {
                StartedWithoutConsole = false;
            }
        }

        /// <summary>
        /// If the process has a console attached to it, it will be detached and no longer visible. Writing to the System.Console is still possible, but no output will be shown.
        /// </summary>
        public static void Hide()
        {
            if (HasConsole)
            {
                FreeConsole();
            }
        }
    }
}

This is redirecting the output to an already present console window e.g. Windows Terminal and exits or creates a new console window and waits for a key press to exit.
Also a .bat-file is possible (creates new window and exits):

start.bat

@echo off
rem first parameter is the window title
start "AnnoDesigner" "%~dp0\AnnoDesigner.exe" --help

[FroggieFrog]

Just to keep feedback in one place (copied from discord):

• The Parser requires a couple of new classes ...Args which are all contained in a single .cs file. This is not very common in C#. I think there is a guideline to put every class in its own file.
• The ArgumentParser.cs mixes multiple concerns in one file. The actual models (...Args), helper models (...Exception) and the logic (Parse(...)).

[Atria1234]

The AnnoDesigner.exe is still executed asynchronously, so if I invoke it from a cmd.exe I'll get prompt back and then the output is put there. This is how the console looks after running AnnoDesigner.exe --help and pressing Enter only once (the blanked parts are the current working directory part of the command line prompt):

Same for powershell

I'll try to play with this, but I'm worried the fact that AD is WPF application will prevent this from working like command line process.

[Atria1234]

Only way I see to make this work as a console app is to make it a console app which starts a WPF app if command line arguments are passed to do so.

Problem that when running AD from cmd or powershell would now be synchronous is not that much of a problem I think.

The main problem is that when running the AD from Windows explorer/whatever will open console by default. I could hide it as first thing after the app starts, but you still might see the console show up for couple of miliseconds. But seeing console blink before some program starts feels very mischievous.

Bottom line: I don't know if it is a good idea to sacrifice AD being windows app for this fuctionality.

[FroggieFrog]

TL;DR: It doesn't matter.

This seems to be a problem of the used console and not a problem of the used shell. See this blog for an explenation.
As seen in my images above it "just works" with the console "Windows Terminal".

Also it is a feature for "power users" only I like to think. So even if there are some issues with the order of output on the console, it doesn't really matter. Because the user wants to export an image and therefor will likely create a script. This script will be run like "fire and forget". So I think the output order is not that of an issue.

This is my point of view. Do you think the order of output is that important?

[Atria1234]

I think power users would be most confused about this, because the program doesn't behave like normal program. And if you are running a script you also might not be bothered by bunch of consoles to appear.

In other words, it also doesn't matter IMHO. So if you really think it would be better to have it reuse parent console, then so be it!

[FroggieFrog]

Things that are missing (in my opinion) for a merge:

• example of a *.bat file for exporting a layout to an image
• output to log files (e.g. used parameters, errors when parsing arguments)
• output to console if export of image is finished

I'm working on the first two items of the list and getting familiar with the CommandLineParser library.
Also I'm testing the export with various parameters.

[FroggieFrog]

@Atria1234

I'm also planning to add additional option "UseUserSettings" to use settings in AppSettings.

I would expect that the switch useUserSettings would be applied first and provided switches afterwards.
This should be possible, because default values are null.
So if the UserSetting is ShowIcons=false and the provided switches are --useUserSettings --showIcons=true the icons would be present in the image.
Do you agree?

Maybe a name like useAppSettings would fit more (even if technically they are UserSettings).

Do I miss any settings apart from the one below?

_appSettings.RenderSelectionHighlightsOnExportedImageValue
_appSettings.ShowGrid
_appSettings.ShowHarborBlockedArea
_appSettings.ShowIcons
_appSettings.ShowInfluences
_appSettings.ShowLabels
_appSettings.ShowPanorama
_appSettings.UseCurrentZoomOnExportedImageValue
_appSettings.ShowTrueInfluenceRange
_appSettings.StatsShowStats

[Atria1234]

Well, you found more settings than me 😄 But I'd still call it useUserSettings because user doesn't care that we have it named appSettings in the soruce code.

Also there is still the question if properties which default to true should be renamed to better show it in their name.

[FroggieFrog]

I'm in the process of implementing the useUserSettings (you won) and found out, that all [Option]s on ExportArgs need to remove the DefaultValue field to work properly.
It is necessary to check the provided arguments with x.HasValue.

Some code I already came up with:

        public CanvasRenderSetting GetRenderSettings(ExportArgs exportArgs)
        {
            //init with default values and overwrite later if necessary
            var result = new CanvasRenderSetting
            {
                GridSize = Constants.GridStepDefault,
                RenderGrid = true,
                RenderHarborBlockedArea = false,
                RenderIcon = true,
                RenderInfluences = false,
                RenderLabel = true,
                RenderPanorama = false,
                RenderStatistics = true,
                RenderTrueInfluenceRange = false,
                RenderVersion = true
            };

            //Use current user settings?
            if (exportArgs is not null && exportArgs.UseAppSettings.HasValue && exportArgs.UseAppSettings.Value)
            {
                result.GridSize = _appSettings.UseCurrentZoomOnExportedImageValue ? AnnoCanvas.GridSize : null;
                result.RenderGrid = _appSettings.ShowGrid;
                result.RenderHarborBlockedArea = _appSettings.ShowHarborBlockedArea;
                result.RenderIcon = _appSettings.ShowIcons;
                result.RenderInfluences = _appSettings.ShowInfluences;
                result.RenderLabel = _appSettings.ShowLabels;
                result.RenderPanorama = _appSettings.ShowPanorama;
                result.RenderStatistics = _appSettings.StatsShowStats;
                result.RenderTrueInfluenceRange = _appSettings.ShowTrueInfluenceRange;
                result.RenderVersion = true;
            }

            //Any settings provided via command line?
            if (exportArgs is not null)
            {
                if (exportArgs.GridSize.HasValue)
                {
                    result.GridSize = Math.Max(Math.Min(exportArgs.GridSize.Value, Constants.GridStepMax), Constants.GridStepMin);
                }

                if (exportArgs.RenderGrid.HasValue)
                {
                    result.RenderGrid = exportArgs.RenderGrid.Value;
                }

                if (exportArgs.RenderHarborBlockedArea.HasValue)
                {
                    result.RenderHarborBlockedArea = exportArgs.RenderHarborBlockedArea.Value;
                }

                if (exportArgs.RenderIcon.HasValue)
                {
                    result.RenderIcon = exportArgs.RenderIcon.Value;
                }

                if (exportArgs.RenderInfluences.HasValue)
                {
                    result.RenderInfluences = exportArgs.RenderInfluences.Value;
                }

                if (exportArgs.RenderLabel.HasValue)
                {
                    result.RenderLabel = exportArgs.RenderLabel.Value;
                }

                if (exportArgs.RenderPanorama.HasValue)
                {
                    result.RenderPanorama = exportArgs.RenderPanorama.Value;
                }

                if (exportArgs.RenderStatistics.HasValue)
                {
                    result.RenderStatistics = exportArgs.RenderStatistics.Value;
                }

                if (exportArgs.RenderTrueInfluenceRange.HasValue)
                {
                    result.RenderTrueInfluenceRange = exportArgs.RenderTrueInfluenceRange.Value;
                }

                if (exportArgs.RenderVersion.HasValue)
                {
                    result.RenderVersion = exportArgs.RenderVersion.Value;
                }
            }

            return result;
        }

[Atria1234]

I really don't like how you have to specify the value of the boolean options with these nullable options

[FroggieFrog]

Ok, I'm not a fan of mixing the parameters (Hide... and Render...), but that seems to be a limitaion of the library.
In the end I think we need to wait for user feedback on that feature, so I'm fine with it.

But I think my above code needs to be used, because at the moment I'm not able to execute following command:

.\AnnoDesigner.exe export --useUserSettings --border=2 --hideIcon "C:\path\to\layout\file.ad" "C:\path\to\exported\image.png"

If the UserSettings have ShowIcons=true, than the resulting image also have the icons. The parameter --hideIcon is ignored.

[Atria1234]

Maybe the "use user settings" should be the default while also having two options ("RenderX" and "HideX") for each render settings which would override the user settings. I really hate forcing users to explicitly specify value of boolean option

[FroggieFrog]

Yes, but this seems to be an issue of the library.
If you know a better one, feel free to make a new PR.

[Atria1234]

Switched to System.CommandLine as proposed. Only thing we lost are usage examples.

Otherwise it is now possible to set boolean option just by mentioning it in the arguments, while also possible to set it to false. Also the render settings from user settings are overriden by the individual render options even if useUserSettings is enabled.

[FroggieFrog]

@Atria1234 You are crazy. All that work is awesome!
I will just add/change a couple of things:

• abstract FileSystem
• adjust tests
• adjust doc
• use some nice features of the lib (e.g. only existing fie paths)

[Atria1234]

I started working on the list before I noticed you were already working on it. 😄

Nice, I wasn't aware this library had this support for files.

[FroggieFrog]

@Atria1234 Could you check again if it still meets your expectations?

[Atria1234]

Looks good to me, thanks for polishing it ❤️

[FroggieFrog]

There was a comment on discord about the first start of the app:

There is just one small thing: If AD is used for the first time it prompts for a language selection. This also happens if I just export an image. In consequence, users suddenly see a language selection dialog when the preview is generated.

So will double check this and see if this can be solved.

[Atria1234]

Clean solution would be to separate business logic from rendering and export the image without even creating the MainWindow instance. This is however tricky, because of all those stuff we are initializing in MainWindow and AnnoCanvas.

The quick and dirty solution would be to just skip openning the Welcome window, but who knows if something will fail later if no language is not selected. Actually I'm pretty sure something will fail.

[FroggieFrog]

For better context and easier implementation the behavior of language selection is tracked in issue #414.