UI Proposal for Code Collaborator settings

[Friedrich Brunzema]

Attached find a proposal screenshot. This would increase the vertical size of the dialog by one "entry". The idea is you could set the settings here, and if configured the extra context menu would show.

------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3057301

To unsubscribe from this discussion, e-mail: [dev-uns...@tortoisesvn.tigris.org].

[Friedrich Brunzema]

More info - formum access via mail does not seem to work...


On Fri, Jun 7, 2013 at 9:22 PM, Stefan Küng <torto...@gmail.com> wrote:

On 07.06.2013 21:17, friedrich brunzema wrote:

I could also detect and show this settings pg only when installed.
Problem with the
Ini file is how to set encrypted passwords.
Even on multiple projects, the Id and pw are not likely different.
Would you more comfortable with proceeding like this?


Not sure what problem you see storing encrypted passwords in an ini file - it's not much different than storing them in the registry as you do now?

If you don't mind, I'll revert your change for now because it's not finished yet and I'd like to create the 1.8 branch.


Ok, I've reverted your changes for now.
And I've added crypt methods to the string utilities so you don't have to misuse the creds class to encrypt your data.
I've also moved your class to separate files.

Now, as for where to store and get the required information:

PathToCollabGui: is that really necessary? I mean you could just request that every user put that path to the PATH environment variable and you're done.
RepoUrl: is that different from the repo root url? If yes is there maybe another way to get the url?

CollabUser/Password, SvnUser/Password: why not just show a dialog asking for that information the first time CodeCollaboratorInfo::GetCommandLineArguments is called and those strings are empty? Then you can store that information wherever you want.

Enabling the whole feature would be done using a project property so projects who don't use it won't even see it.

Stefan

--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net

------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3057319

[Friedrich Brunzema]

Thinking about it some more, I agree with not putting the code collaborator settings with the TSVN settings. [It was still fun to do, as it made me remember MFC stuff that I had not seen for 10 years].

What really needs to be persisted is the two user names and passwords (with the passwords encrypted). Thanks to Stefan for upgrading the CStringsUtils - its really where that code belongs!

I don't like the idea putting the usernames into SVN properties and the passwords in an .ini file, as this separates the info. I would much rather like to keep these code collaborator settings all together.

So lets go, as Stefan suggested with a standard windows .ini file located in the %appdata%\TortoiseSvn that stores the 4 aforementioned parameters. This means that its a per-windows user setting. If the collabgui.exe file is found in the standard location (x86) and (x64), then the Add to Code Collaborator menu item is shown when right clicking the revisions in the LogDialog. No app found, no menu item. [If it is not in the standard location, they are out of luck] If the .ini file does not exist, the user is prompted with a Modal Dialog that lets him/her enter the data. The dialog is not shown if the .ini file exists, unless the user presses control when invoking the menu. That way the user has a way to update the usernames and encrypted passwords.

Unfortunately, the unencrypted passwords along with the usernames are passed on the command line to collabgui - which is a bad design. Using tools like Sysinternals Process explorer lets you see the commandline, exposing the passwords. But I agree that it is better to do due diligence when storing passwords - ie encrypt them. I will point this out to the Code Collaborator authors and ask them for a better way to do this.

If anyone has concerns with this approach or further suggestions, let me by posting to this thread.

Best,

Friedrich

------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3057320

[Friedrich Brunzema]

I could also detect and show this settings pg only when installed. Problem with the
Ini file is how to set encrypted passwords.
Even on multiple projects, the Id and pw are not likely different.
Would you more comfortable with proceeding like this?

Friedrich

---

From: Stefan Küng
Sent: ‎2013-‎06-‎07 15:01
To: brun...@yahoo.com
Subject: Fwd: UI Proposal for Code Collaborator settings

Something is seriously wrong with the mailing list, my posts don't seem to show up there...

---------- Forwarded message ----------
From: Stefan Küng <torto...@gmail.com>
Date: Fri, Jun 7, 2013 at 8:57 PM
Subject: Re: UI Proposal for Code Collaborator settings
To: dev <d...@tortoisesvn.tigris.org>

On 07.06.2013 03:50, Friedrich Brunzema wrote:

Attached find a proposal screenshot. This would increase the vertical
size of the dialog by one "entry".  The idea is you could set the
settings here, and if configured the extra context menu would show.

Uh, that's not good. Imagine we extend this feature to other tools as well. Would every tool get its own settings page?
And this isn't really "hide it if it's not used".
I suggest implementing this by just reading an ini file from the working copy root when you need the data. If the ini file exists, use it and show the context menu. If it doesn't, then everything is hidden.
Or use project properties (minus username/password).

Stefan

--
       ___
  oo  // \\      "De Chelonian Mobile"
 (_,\/ \_/ \     TortoiseSVN

   \ \_/_\_/>    The coolest interface to (Sub)version control
   /_/   \_\     http://tortoisesvn.net

[Stefan Küng]

On 07.06.2013 21:17, friedrich brunzema wrote:

> I could also detect and show this settings pg only when installed.
> Problem with the
> Ini file is how to set encrypted passwords.
> Even on multiple projects, the Id and pw are not likely different.
> Would you more comfortable with proceeding like this?

Not sure what problem you see storing encrypted passwords in an ini file
- it's not much different than storing them in the registry as you do now?

If you don't mind, I'll revert your change for now because it's not
finished yet and I'd like to create the 1.8 branch.

Stefan

--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest interface to (Sub)version control
/_/ \_\ http://tortoisesvn.net

------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3057336

[Stefan Küng]

On 07.06.2013 03:50, Friedrich Brunzema wrote:

> Attached find a proposal screenshot. This would increase the vertical
> size of the dialog by one "entry". The idea is you could set the
> settings here, and if configured the extra context menu would show.

Uh, that's not good. Imagine we extend this feature to other tools as
well. Would every tool get its own settings page?
And this isn't really "hide it if it's not used".
I suggest implementing this by just reading an ini file from the working
copy root when you need the data. If the ini file exists, use it and
show the context menu. If it doesn't, then everything is hidden.
Or use project properties (minus username/password).

Stefan

--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest interface to (Sub)version control
/_/ \_\ http://tortoisesvn.net

------------------------------------------------------

http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3057576

[Stefan Küng]

On Fri, Jun 7, 2013 at 9:22 PM, Stefan Küng <torto...@gmail.com> wrote:

On 07.06.2013 21:17, friedrich brunzema wrote:

I could also detect and show this settings pg only when installed.
Problem with the
Ini file is how to set encrypted passwords.
Even on multiple projects, the Id and pw are not likely different.
Would you more comfortable with proceeding like this?

Not sure what problem you see storing encrypted passwords in an ini file - it's not much different than storing them in the registry as you do now?

If you don't mind, I'll revert your change for now because it's not finished yet and I'd like to create the 1.8 branch.

Ok, I've reverted your changes for now.

And I've added crypt methods to the string utilities so you don't have to misuse the creds class to encrypt your data.

I've also moved your class to separate files.

Now, as for where to store and get the required information:

PathToCollabGui: is that really necessary? I mean you could just request that every user put that path to the PATH environment variable and you're done.

RepoUrl: is that different from the repo root url? If yes is there maybe another way to get the url?

CollabUser/Password, SvnUser/Password: why not just show a dialog asking for that information the first time CodeCollaboratorInfo::GetCommandLineArguments is called and those strings are empty? Then you can store that information wherever you want.

Enabling the whole feature would be done using a project property so projects who don't use it won't even see it.

Stefan

--

       ___
  oo  // \\      "De Chelonian Mobile"
 (_,\/ \_/ \     TortoiseSVN

[Stefan Küng]

On 08.06.2013 01:58, Friedrich Brunzema wrote:
> Thinking about it some more, I agree with not putting the code
> collaborator settings with the TSVN settings. [It was still fun to
> do, as it made me remember MFC stuff that I had not seen for 10
> years].
>
> What really needs to be persisted is the two user names and passwords
> (with the passwords encrypted). Thanks to Stefan for upgrading the
> CStringsUtils - its really where that code belongs!
>
> I don't like the idea putting the usernames into SVN properties and
> the passwords in an .ini file, as this separates the info. I would
> much rather like to keep these code collaborator settings all
> together.
>
> So lets go, as Stefan suggested with a standard windows .ini file
> located in the %appdata%\TortoiseSvn that stores the 4 aforementioned
> parameters. This means that its a per-windows user setting. If the
> collabgui.exe file is found in the standard location (x86) and (x64),
> then the Add to Code Collaborator menu item is shown when right
> clicking the revisions in the LogDialog. No app found, no menu item.
> [If it is not in the standard location, they are out of luck] If the
> .ini file does not exist, the user is prompted with a Modal Dialog
> that lets him/her enter the data. The dialog is not shown if the
> .ini file exists, unless the user presses control when invoking the
> menu. That way the user has a way to update the usernames and
> encrypted passwords.

looks good.
But just to clarify: %appdata% is not per-windows but per-user: it's the
roaming profile for each user.

And if you ask for usernames/passwords with a dialog when it's first
needed, then there's no need to have an ini file at all: you can just
store that in the registry as you do now.

> Unfortunately, the unencrypted passwords along with the usernames are
> passed on the command line to collabgui - which is a bad design.
> Using tools like Sysinternals Process explorer lets you see the
> commandline, exposing the passwords. But I agree that it is better
> to do due diligence when storing passwords - ie encrypt them. I will
> point this out to the Code Collaborator authors and ask them for a
> better way to do this.

Just to be clear: it's not really bad. If you have the rights to read
another processes command line then you also have the rights to decrypt
the data from e.g. the registry where you store it encrypted.
If a bad app makes it that far on your system, then it could do much
more and worse.
It's only bad because it's visible in plain text with standard tools.

Stefan

--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN

\ \_/_\_/> The coolest interface to (Sub)version control

/_/ \_\ http://tortoisesvn.net

------------------------------------------------------

http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3057627

[Friedrich Brunzema]

Couple of ideas regarding this functionality:
1. The detection logic for CC is broken on x64 systems, as CC has both an x86 and x64 client. Result is that the program is always installed in %program files%\..\ and never in Program Files (x86). I will fix this.

2. There is a usability issue for single commits. Currently, the user has to
1. Commit his/her code
2. Do a SVN->Log
3. Right Click the new revision just checked in
4. Choose Create Code Collaborator review.

I propose adding a way to automatically create a code review if:
1. Collaborator is installed
2. Collaborator is configured in the registry
3. User presses Ctrl while clicking OK
This would do the normal commit, and upon success get the commit rev# and do the normal create review functionality.

An alternative to (3) is to have a checkbox between the Keep Locks checkbox and the Show Log button. This would probably entail increasing the min width of the dialog by 1-2cm to make room for a "Create Review" checkbox. The wider dialog limits would only apply if the program is installed and the checkbox would also only show if installed.
The good: More visibility, less hacky then control+OK
The bad: A bit more complexity in the commit dlg.

Right now my gut feeling is not to do the UI option but to use the Ctrl+OK

Thoughts?

------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3058029

[Stefan Küng]

On 13.06.2013 23:30, Friedrich Brunzema wrote:
> Couple of ideas regarding this functionality:
> 1. The detection logic for CC is broken on x64 systems, as CC has both an x86 and x64 client. Result is that the program is always installed in %program files%\..\ and never in Program Files (x86). I will fix this.
>
> 2. There is a usability issue for single commits. Currently, the user has to
> 1. Commit his/her code
> 2. Do a SVN->Log
> 3. Right Click the new revision just checked in
> 4. Choose Create Code Collaborator review.
>
> I propose adding a way to automatically create a code review if:
> 1. Collaborator is installed
> 2. Collaborator is configured in the registry
> 3. User presses Ctrl while clicking OK
> This would do the normal commit, and upon success get the commit rev# and do the normal create review functionality.

> An alternative to (3) is to have a checkbox between the Keep Locks checkbox and the Show Log button. This would probably entail increasing the min width of the dialog by 1-2cm to make room for a "Create Review" checkbox. The wider dialog limits would only apply if the program is installed and the checkbox would also only show if installed.
> The good: More visibility, less hacky then control+OK
> The bad: A bit more complexity in the commit dlg.
>
> Right now my gut feeling is not to do the UI option but to use the Ctrl+OK

That's actually what either a pre-commit hook script is for or an issue
tracker plugin. No need to mess with the TSVN code for this.

You could write a client-side hook script:
http://tortoisesvn.net/docs/release/TortoiseSVN_en/tsvn-dug-settings.html#tsvn-dug-settings-hooks

in this situation, I would recommend using the post-commit hook, since
you don't want to mess with CC if the commit fails but only if the
commit succeeded. That script is called after the commit is finished so
you can mess with CC at a time when all the info about the commit is
already available.

If you need something more complicated, you can implement an issue
tracker plugin:
http://tortoisesvn.net/docs/release/TortoiseSVN_en/tsvn-ibugtraqprovider-2.html

The method CheckCommit() is called right before the commit dialog is
closed and the commit is about to start. You have there the ability to
stop the commit dialog from closing and show the user e.g. an error
message if something is missing or wrong.

The method OnCommitFinished() is called after the commit has finished
and you could then add a review task for that revision to CC.

Stefan

--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest interface to (Sub)version control
/_/ \_\ http://tortoisesvn.net

------------------------------------------------------

http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3058083

[Friedrich Brunzema]

Stefan Wrote:

>That's actually what either a pre-commit hook script is for or an issue
>tracker plugin. No need to mess with the TSVN code for this.

>...

I will explore that further, the less changes to the code, the better.  Hopefully there is a way to do a post-commit hook in an optional way - I will read the docs...

F.

[Friedrich Brunzema]

Stefan wrote:
>You could write a client-side hook script:
>http://tortoisesvn.net/docs/release/TortoiseSVN_en/tsvn-dug-settings.html#tsvn-dug-settings-hooks

>in this situation, I would recommend using the post-commit hook, since
>you don't want to mess with CC if the commit fails but only if the
>commit succeeded. That script is called after the commit is finished so
>you can mess with CC at a time when all the info about the commit is
>already available.

There is enough info there to do it with a client-side script -- but it was
tempting to do it in the CommitDlg because I already have C++ infrastructure.
But I agree with you that it does not belong the commit dialog if it can be avoided.
I will build a little UI-less MFC Dialog-based app with the string decryption, reg
classes - actually I don't even need UI.  I will check the parameters and see
if Ctrl is still pressed - if so, I launch the collaborator program with the revision#.
One problem:  I need to convert the current working directory into a URL - how
would one go about doing that?

If I build such a little program and borrow TSVN code, I think the license tells me that I have to publish
the source, right?  Any suggestions as to where to publish it?  GitHub?  Recently looked at Git,
powerful, but not very intuitive.  The decentralized model works well for OpenSource, but not
as well for corporate organizations who like control.

Thanks for pointing the client-side hook script out to me -- shows you how
little I know of TSVN...

Best,

Friedrich

[Stefan Küng]

On 15.06.2013 06:13, Friedrich Brunzema wrote:
>
> Stefan wrote:
> >You could write a client-side hook script:

> >http://tortoisesvn.net/docs/release/TortoiseSVN_en/tsvn-dug-settings.html#tsvn-dug-settings-hooks <http://tortoisesvn.net/docs/release/TortoiseSVN_en/tsvn-dug-settings.html#tsvn-dug-settings-hooks>

>
> >in this situation, I would recommend using the post-commit hook, since
> >you don't want to mess with CC if the commit fails but only if the
> >commit succeeded. That script is called after the commit is finished so
> >you can mess with CC at a time when all the info about the commit is
> >already available.
>
> There is enough info there to do it with a client-side script -- but it was
> tempting to do it in the CommitDlg because I already have C++
> infrastructure.

You can still implement your hook script a as C++ application if you want.

> But I agree with you that it does not belong the commit dialog if it can
> be avoided.
> I will build a little UI-less MFC Dialog-based app with the string
> decryption, reg
> classes - actually I don't even need UI. I will check the parameters
> and see
> if Ctrl is still pressed - if so, I launch the collaborator program with
> the revision#.

Do NOT use Ctrl to determine anything: you can close most TSVN dialogs
directly using Ctrl+Enter - that's useful for all those dialogs where
the focus is on an input edit control where simply pressing Enter will
add a newline instead of closing the dialog.

So all those people who use Ctrl+Enter to fast close the dialog will
always get your special treatment if you check for that.

> One problem: I need to convert the current working directory into a URL
> - how
> would one go about doing that?

If you implement an issue tracker plugin, you get the url of the common
root of all committed files in CheckCommit().

>
> If I build such a little program and borrow TSVN code, I think the
> license tells me that I have to publish
> the source, right? Any suggestions as to where to publish it? GitHub?
> Recently looked at Git,
> powerful, but not very intuitive. The decentralized model works well
> for OpenSource, but not
> as well for corporate organizations who like control.

You don't have to publish it: you only have to provide the source code
if someone asks and of course you have to indicate that somewhere in
your about box and/or website.

Stefan

--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest interface to (Sub)version control
/_/ \_\ http://tortoisesvn.net

------------------------------------------------------

http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3058154

[Friedrich Brunzema]

Stefan wrote:

>You can still implement your hook script a as C++ application if you want.

That's exactly what I am doing right now.

>Do NOT use Ctrl to determine anything: you can close most TSVN dialogs
>directly using Ctrl+Enter - that's useful for all those dialogs where
>the focus is on an input edit control where simply pressing Enter will
>add a newline instead of closing the dialog.
>So all those people who use Ctrl+Enter to fast close the dialog will
>always get your special treatment if you check for that.

Nice feature!  Did not know about that one either.  I could  use ALT instead.

>If you implement an issue tracker plugin, you get the url of the common
>root of all committed files in CheckCommit().

We're already using an issue tracker plugin.  Can you use multiple issue trackers for one WC?

Best,

Friedrich

[Stefan Küng]

No, but how about extending the one you already use?

Stefan

--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest interface to (Sub)version control
/_/ \_\ http://tortoisesvn.net

------------------------------------------------------

http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3058291

[Friedrich Brunzema]

Stefan wrote:
> No, but how about extending the one you already use?

Exactly my thoughts. I have modified the Tortoise Jira plugin (see screenshot). If collaborator is installed, it shows the new checkbox.

A hook script could also be done (applicable for those that don't use Jira), but we'll leave that as an exercise to the reader ;-) I can certainly share the code I have - plan publish that on GitHub.

This brings that part to a conclusion, I hope.

------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3058400