Implement filling Webforms in HTTP/HTTPS plugin #95
Conversation
Lillecarl
force-pushed
the
PR_HTTP_Forms
branch
from
2dbc83c
to
949c3f0
Compare
Lillecarl
force-pushed
the
PR_HTTP_Forms
branch
from
949c3f0
to
723838b
Compare
Lillecarl
changed the title
|
Please add missing unit tests, fix WebProtocols_CreateControls_ReturnsEmpty test. |
|
Considering i don't know how to run the tests, i hope this is what you're looking for :) |
|
Still two tests are failing. I am going to have a look my self. So we can merge it. |
|
To fix the tests:
|
| @@ -6,6 +6,8 @@ public class KnownConnectionConstants | |||
|
|
|||
| public const int HTTPPort = 80; | |||
|
|
|||
| public const int HTTPSPort = 443; | |||
There was a problem hiding this comment.
There should be no reason why to move it here, until there is another usage than tests. Leave it in https plugin.
| @@ -37,12 +41,17 @@ public ProtocolOptions CreateOptions() | |||
|
|
|||
| public Image GetIcon() | |||
| { | |||
| return HttpConnectionPlugin.TreeIconHttp; | |||
| return TreeIconHttps; | |||
There was a problem hiding this comment.
Why is the icon moved to this plugin and not referencing the original icon. It looks like a bug?
There was a problem hiding this comment.
internal static readonly Image TreeIconHttps = Resources.treeIcon_http; I added this to "sync" the two plugins to be as equal as possible, could absolutely be reverted :)
| public string OptionalID { get; set; } | ||
| public string OptionalValue { get; set; } | ||
| public string SubmitID { get; set; } | ||
| public bool EnableHTMLAuth { get; set; } |
There was a problem hiding this comment.
Why is the EnableHtmlAuth separated from EnableFormsAuth, what is the difference. I would expect one dropdown to select authentication method. Or is the purpose as fallback authentication?
There was a problem hiding this comment.
Added both to be able to keep both enabled by default, to ease for users (Not having to chose by default).
| { | ||
| UrlConverter.UpdateFavoriteUrl(context.Favorite, context.ConfigFavorite.Url); |
There was a problem hiding this comment.
Why are this property conversions removed? The property is still there, so it needs to be converted.
There was a problem hiding this comment.
I'll look into it :)
|
|
||
| internal class TerminalsWebExport | ||
| { | ||
| public static void ExportOptions(ref IExportOptionsContext context) |
There was a problem hiding this comment.
Why are you calling the context using ref?
| } | ||
| } | ||
|
|
||
| internal class TerminalsHTTPSExport : ITerminalsOptionsExport |
There was a problem hiding this comment.
No need to have two exporters, if they do the same. We may implement one for both protocols. See ConnectionManager.IsProtocolWebBased.
There was a problem hiding this comment.
Correcting this :)
|
Updated to try and fit your comments. It might still not be perfect for your style though. We'll see :) |
|
Please add missing tests to cover what is not automatically tested. Otherwise i will fix the issues you dint comment. |
|
If there's anything you want fixed, feel free to push to my branch too, you should have write access to the pullrequested branches :) My reasoning behind instancing the icon twice and moving the https port definition is only to keep consistency within the codebase. Hope to see this if not merged, implemented :) It's a killer feature! :) |
|
If you want, we can rebase this branch on master and rewrite the history too :) Contributed to a project awhile ago where a pretty gitlog was important. |
|
How are we doing on this? I'm not sure which changes i've missed, please rereview this. |
|
Go through all comments for each file, mainly:
|
|
@jirkapok It's the wrong way around, but i'd love it if you helped me cleanup the last bits as you'd like them. I think by GitHub design you've got write access to the pullrequest branch. The tests i think i implemented, i'm not sure what's missing. And upgrades i know not how to do. Best Regards |
|
Based on project resurection, i will cherry pick these changes and implement them in 46-putty branch. |
|
I'm glad to hear, I don't have any Windows machines anymore so I can't assist, but I suspekt you've got this 😄 |
Support for filling forms in HTTP and HTTPS plugins