Re: [tortoisesvn] r24360 committed - WhiteSpace-Fix handling:...

[Oto BREZINA]

On 12. júna 2013 21:39, torto...@googlecode.com wrote:
> Revision: 24360
> Author: tortoisesvn
> Date: Wed Jun 12 12:39:24 2013
> Log: WhiteSpace-Fix handling:
> * use a separate dialog for the setup: reusing the same dialog should be
> avoided
yap but it keep things on one place I was not very convinced about its
rightness too.
> * never automatically fix: if there is something to fix, always ask first
You asked this :)
I have no access to my machine right now, but reading diif I get
feeling, that any indentation will popup fix before save dialog and
there will be no default set. In fact the other then actual will be
selected or both if mixed.
If I'm mistaken just ignore it. I'll have a look tomorrow anyway.
> * use defines instead of hard coded values
Was in plan once behaiviour get fixed ...
> CDialog::OnInitDialog();
>
> + m_trimRight.SetCheck(trimRight);
> + m_useSpaces.SetCheck(convertTabs);
> + m_useTabs.SetCheck(convertSpaces);
> + m_fixEols.SetCheck(fixEols);
> + m_EOL.EnableWindow(fixEols);

> + if ((nMap & TMERGE_WSF_ASKFIX_TABSPACE) && (convertTabs ||
> convertSpaces))
> + return true;

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

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

[Stefan Küng]

On 12.06.2013 22:11, Oto BREZINA wrote:
> On 12. júna 2013 21:39, torto...@googlecode.com wrote:
>> Revision: 24360
>> Author: tortoisesvn
>> Date: Wed Jun 12 12:39:24 2013
>> Log: WhiteSpace-Fix handling:
>> * use a separate dialog for the setup: reusing the same dialog should be
>> avoided
> yap but it keep things on one place I was not very convinced about its
> rightness too.
>> * never automatically fix: if there is something to fix, always ask first
> You asked this :)

I think that was a misunderstanding. I only wanted to always ask to fix
EOLs, not fixing them without asking.

> I have no access to my machine right now, but reading diif I get
> feeling, that any indentation will popup fix before save dialog and
> there will be no default set. In fact the other then actual will be
> selected or both if mixed.
> If I'm mistaken just ignore it. I'll have a look tomorrow anyway.

ok, I haven't checked that.

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=3057921

[Oto BREZINA]

On 2013-06-12 22:19, Stefan Küng wrote:
> On 12.06.2013 22:11, Oto BREZINA wrote:
>> On 12. júna 2013 21:39, torto...@googlecode.com wrote:
>>> * never automatically fix: if there is something to fix, always ask first
>> You asked this :)
> I think that was a misunderstanding. I only wanted to always ask to fix
> EOLs, not fixing them without asking.

Here is excerpt from your email "TMerge: fixing inconsistencies on save"
On 2013-06-10 20:05, Stefan Küng wrote:
> The new feature about fixing inconsistencies when saving in TMerge looks promising. But I have a few suggestions:
>
> * instead of deactivating space/tab and tab/space, I would just use a radio button: if there are such inconsistencies always show that radio button and have the user chose what to do instead of trying to figure out what the user might want to do
> * always ask if there are inconsistencies, no matter what the state was on load
>
> this could also be improved so that the settings dialog has options to either never ask to fix a specific inconsistency, or even always fix it without asking (three state: never/always/ask). And this for each of the inconsistencies it can fix.

>
>> I have no access to my machine right now, but reading diif I get
>> feeling, that any indentation will popup fix before save dialog and
>> there will be no default set. In fact the other then actual will be
>> selected or both if mixed.
>> If I'm mistaken just ignore it. I'll have a look tomorrow anyway.
> ok, I haven't checked that.

It works as I expect from code.

Let me elaborate little bit about posibilities so before next rewrite we
will better know what we want to get.

* Eols and tailing whites - can be detected on the fly
* always asked if detected
* always fixed
* fixed with or without asking if was ok on load - error introduced
by editing in T-Merge, T-Merges purposes in first place is to be
diff/merge tool not an edit, so most of this error comes from other
sources so will be present on load anyway
* fixed with or without asking if was ok on "left" view (not sure
about merge)
Now if you start to build map of all possibly useful version its quite
big thing
My favorite would be:
Auto fix if it was ok in left, ask otherwise.

To make it less complex we may add some notification in view title, or
status bar, but it may require lot of processing or very good design.
Lets stick we current approach for a while.

In spaces and tabs conversion it become even more complex.
Your left file may be all space indented and saving file all tabs. Is
this correct or not ? In current implementation it is correct as it is
self homogeniuos.

And then if you ignore left/load state you need set preferred indentation.

All can be implemented but before that we need be clear what we want.

>
> Stefan
>

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

[Stefan Küng]

On 13.06.2013 09:29, Oto BREZINA wrote:

> Let me elaborate little bit about posibilities so before next rewrite we
> will better know what we want to get.
>
> * Eols and tailing whites - can be detected on the fly
> * always asked if detected
> * always fixed
> * fixed with or without asking if was ok on load - error introduced
> by editing in T-Merge, T-Merges purposes in first place is to be
> diff/merge tool not an edit, so most of this error comes from other
> sources so will be present on load anyway
> * fixed with or without asking if was ok on "left" view (not sure
> about merge)
> Now if you start to build map of all possibly useful version its quite
> big thing
> My favorite would be:
> Auto fix if it was ok in left, ask otherwise.

No, auto-fixing without asking is very very bad.

Just detect and ask if necessary.

> To make it less complex we may add some notification in view title, or
> status bar, but it may require lot of processing or very good design.
> Lets stick we current approach for a while.
>
> In spaces and tabs conversion it become even more complex.
> Your left file may be all space indented and saving file all tabs. Is
> this correct or not ? In current implementation it is correct as it is
> self homogeniuos.

When doing such fixing, the views have to be handled completely
independent of each other. Why would the left view have anything to do
with how the right view is saved? Doesn't make any sense to me and IMHO
would make things just unnecessarily complicated. And if it's
complicated, the users also won't understand why sometimes it asks to
fix and sometimes not.

> And then if you ignore left/load state you need set preferred indentation.

I also don't understand what you want to do exactly with indentation.
Fixing space/tabs I understand, but indentation? Fixing that is not the
job of a diff tool at all because it's basically a formatting change and
depends on file type, language, ...

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=3058020

[Oto BREZINA]

Few months ago you stand on T-Merge is diff tool where right is editable
output and left is source.
I consider left as source thing in MOST cases so can be good source of
expected tab/spaces style.
File on load may be damaged by others editing tools (settings), and
T-Merge as last station before commit can warning you that you are about
to change that style.

This wont solve fixing left file though.

>
>> And then if you ignore left/load state you need set preferred indentation.
> I also don't understand what you want to do exactly with indentation.
> Fixing space/tabs I understand, but indentation? Fixing that is not the
> job of a diff tool at all because it's basically a formatting change and
> depends on file type, language, ...

Indentation in this context meas use spaces or tabs to indent, sorry if
this was confusing.
>
> Stefan

Oto
>

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

[Stefan Küng]

What does that have to do with keeping the views separate?
I never said that this fixing should only work for one view. I said that
fixing one view has nothing to do with any other view that's visible or
loaded.

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=3058085

[Oto BREZINA]

I'm not going deeper here there is no sense in that.

Just: not long ago T-Merge philosofy was Left is source, right is
target/edit (in diff). Using this : formating left is more possible
proper format according repository style against right one.
e.g. you are diffing .rc file where spaces are on line ends, T-Merge can
see it is that way on repository, so will offer to fix it. But if you
indent by tabs it can detect it ...
That's all I meant. Maybe it was incorrect deduction but it worked in my
mind.


>
> Stefan
Oto

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

[Stefan Küng]

On 14.06.2013 18:01, Oto BREZINA wrote:

> Just: not long ago T-Merge philosofy was Left is source, right is
> target/edit (in diff). Using this : formating left is more possible
> proper format according repository style against right one.
> e.g. you are diffing .rc file where spaces are on line ends, T-Merge can
> see it is that way on repository, so will offer to fix it. But if you
> indent by tabs it can detect it ...
> That's all I meant. Maybe it was incorrect deduction but it worked in my
> mind.

But who's to say that the left (repository, working base, ...) is
correct? I'd say in most situations it is not correct either if the
right view is not.
So really the views should be treated separately and only the one to
save (if we keep the on-save detection) or the active/editable view
should be considered, no other view should be mixed in.

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=3058096

[Oto BREZINA]

On 2013-06-14 18:48, Stefan Küng wrote:
> On 14.06.2013 18:01, Oto BREZINA wrote:
>
>> Just: not long ago T-Merge philosofy was Left is source, right is
>> target/edit (in diff). Using this : formating left is more possible
>> proper format according repository style against right one.
>> e.g. you are diffing .rc file where spaces are on line ends, T-Merge can
>> see it is that way on repository, so will offer to fix it. But if you
>> indent by tabs it can detect it ...
>> That's all I meant. Maybe it was incorrect deduction but it worked in my
>> mind.
> But who's to say that the left (repository, working base, ...) is
> correct? I'd say in most situations it is not correct either if the
> right view is not.
> So really the views should be treated separately and only the one to
> save (if we keep the on-save detection) or the active/editable view
> should be considered, no other view should be mixed in.

It not neccessary correct but it is BEST we have droping BEST we are
left with only worse solutions.

Ok I'll remove all "fix on save" stuff.
>
> Stefan
>

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