Re: [tortoisesvn] r24370 committed - Improve space/tab inconsistency detection:...

5 views
Skip to first unread message

Oto BREZINA

unread,
Jun 13, 2013, 3:11:31 PM6/13/13
to d...@tortoisesvn.tigris.org
On 2013-06-13 19:21, torto...@googlecode.com wrote:
> Revision: 24370
> Author: tortoisesvn
> Date: Thu Jun 13 10:21:40 2013
> Log: Improve space/tab inconsistency detection:
> * only if there are consecutive spaces to match a tab is the file marked as
> using spaces
> * only if there are tabs is the file marked as using tabs
> * only if both tabs and spaces are used is the file marked as space/tab
> inconsistent.

While reading this keep in mind I don't want nor meant to attack in any way.
However I don't know what this revision improved over let say r24355.
I thought "fix before save" will be simple improvement over manual
invocation, but seems its getting over us.

r24370:
It detect tabs even after first non space/tab character - that will not
be converted (intentionaly) so offer is not adequate. Same for spaces.
e.g.
1. our line formating as follow"<tab><tab>Call()<few spaces>// make a
call" will offer tab->spaces
2. space indented line "<8 spaces>Indent "<tab>" // will offer spaces->tab
one line with two tabs and other with 8 spaces offers BOTH on save!

If there is one space between two tabs, If will not help me to clean it
up anymore
If both tabs and spaces are used I get convert tabs to spaces (I preffer
tabs) no way to configure anymore.

I agree tri-state config was a little bit confusing but coverted 98% of
cases if you understand it, now I get under 50%.

Oto

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

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

Stefan Küng

unread,
Jun 14, 2013, 11:13:05 AM6/14/13
to d...@tortoisesvn.tigris.org
On 13.06.2013 21:11, Oto BREZINA wrote:
> On 2013-06-13 19:21, torto...@googlecode.com wrote:
>> Revision: 24370
>> Author: tortoisesvn
>> Date: Thu Jun 13 10:21:40 2013
>> Log: Improve space/tab inconsistency detection:
>> * only if there are consecutive spaces to match a tab is the file marked as
>> using spaces
>> * only if there are tabs is the file marked as using tabs
>> * only if both tabs and spaces are used is the file marked as space/tab
>> inconsistent.
>
> While reading this keep in mind I don't want nor meant to attack in any way.
> However I don't know what this revision improved over let say r24355.
> I thought "fix before save" will be simple improvement over manual
> invocation, but seems its getting over us.
>
> r24370:
> It detect tabs even after first non space/tab character - that will not
> be converted (intentionaly) so offer is not adequate. Same for spaces.

haven't checked that part of the code. So you're saying that only
leading tabs/spaces are converted but not if they're in the middle of
the line?
That's seems pretty useless to me, so I'm wondering why this was
intentional?

> e.g.
> 1. our line formating as follow"<tab><tab>Call()<few spaces>// make a
> call" will offer tab->spaces
> 2. space indented line "<8 spaces>Indent "<tab>" // will offer spaces->tab
> one line with two tabs and other with 8 spaces offers BOTH on save!
>
> If there is one space between two tabs, If will not help me to clean it
> up anymore
> If both tabs and spaces are used I get convert tabs to spaces (I preffer
> tabs) no way to configure anymore.
>
> I agree tri-state config was a little bit confusing but coverted 98% of
> cases if you understand it, now I get under 50%.

I'm starting to wonder whether it's a good idea to even check and ask
the user to fix these. It would be easier to just have buttons in the
ribbon/toolbar to fix whatever needs to be fixed and have the user
execute those manually if needed.

That way the whole detection logic can be removed since I'm pretty sure
it won't work in *all* situations, no matter how good we can make it.

I have used a lot of IDE's and editors, and the ones that fix stuff
automatically I stopped using because they mess things up without me
noticing immediately (yes, sometimes I *need* a space at the end of a
line). The ones that always ask to fix something I either configure to
never do that again or I rarely use them anymore - too annoying.
I use however those frequently that have the commands to fix stuff but I
can do that manually - I know myself if something needs fixing.


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

Oto BREZINA

unread,
Jun 14, 2013, 11:48:22 AM6/14/13
to d...@tortoisesvn.tigris.org
On 2013-06-14 17:13, Stefan Küng wrote:
> On 13.06.2013 21:11, Oto BREZINA wrote:
>>
>> r24370:
>> It detect tabs even after first non space/tab character - that will not
>> be converted (intentionaly) so offer is not adequate. Same for spaces.
> haven't checked that part of the code. So you're saying that only
> leading tabs/spaces are converted but not if they're in the middle of
> the line?
> That's seems pretty useless to me, so I'm wondering why this was
> intentional?
Best suited style for us is indent by tabs allign by spaces (I'm not
going to explain).
We are keeping T-Merge not language aware - that good - but there are
languages when tab is in text in not escaped form but indentation is by
spaces.

Lets call fixing indentation spaces tabs level 1.
And thole line level 2 tools.
>
> I agree tri-state config was a little bit confusing but coverted 98% of
> cases if you understand it, now I get under 50%.
> I'm starting to wonder whether it's a good idea to even check and ask
> the user to fix these. It would be easier to just have buttons in the
> ribbon/toolbar to fix whatever needs to be fixed and have the user
> execute those manually if needed.
>
> That way the whole detection logic can be removed since I'm pretty sure
> it won't work in *all* situations, no matter how good we can make it.
>
> I have used a lot of IDE's and editors, and the ones that fix stuff
> automatically I stopped using because they mess things up without me
> noticing immediately (yes, sometimes I *need* a space at the end of a
> line). The ones that always ask to fix something I either configure to
> never do that again or I rarely use them anymore - too annoying.
> I use however those frequently that have the commands to fix stuff but I
> can do that manually - I know myself if something needs fixing.
As I wrote earlier some statuses (status bar - prefferably clickabel,
ribbon, toolbar) would be great, but I was affraid about proccessing
needed to keep them reasonably actual.
Just note: detection is to help when using commands from context menu too.

Ok lets remove auto fix stuff, commands are in context menu (in first
level only for a moment a I don't see level 2 to be safe).

Whole auto fixing was to get as close to #479 as possible.


Oto

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

Stefan Küng

unread,
Jun 14, 2013, 12:45:52 PM6/14/13
to d...@tortoisesvn.tigris.org
On 14.06.2013 17:48, Oto BREZINA wrote:

>> haven't checked that part of the code. So you're saying that only
>> leading tabs/spaces are converted but not if they're in the middle of
>> the line?
>> That's seems pretty useless to me, so I'm wondering why this was
>> intentional?
> Best suited style for us is indent by tabs allign by spaces (I'm not
> going to explain).

have a look at my tabspace tool:
http://tools.tortoisesvn.net/tabspace.html

it works as I think you want it: using tabs but then spaces to align to
mid-tab positions.
And it does this not just at the beginning of lines but for all tabs/spaces.

>> I agree tri-state config was a little bit confusing but coverted 98% of
>> cases if you understand it, now I get under 50%.
>> I'm starting to wonder whether it's a good idea to even check and ask
>> the user to fix these. It would be easier to just have buttons in the
>> ribbon/toolbar to fix whatever needs to be fixed and have the user
>> execute those manually if needed.
>>
>> That way the whole detection logic can be removed since I'm pretty sure
>> it won't work in *all* situations, no matter how good we can make it.
>>
>> I have used a lot of IDE's and editors, and the ones that fix stuff
>> automatically I stopped using because they mess things up without me
>> noticing immediately (yes, sometimes I *need* a space at the end of a
>> line). The ones that always ask to fix something I either configure to
>> never do that again or I rarely use them anymore - too annoying.
>> I use however those frequently that have the commands to fix stuff but I
>> can do that manually - I know myself if something needs fixing.

> As I wrote earlier some statuses (status bar - prefferably clickabel,
> ribbon, toolbar) would be great, but I was affraid about proccessing
> needed to keep them reasonably actual.
> Just note: detection is to help when using commands from context menu too.

detection is nice, yes. But it's not necessary: you can still have a
command button enabled but when clicked doesn't change anything.

> Ok lets remove auto fix stuff, commands are in context menu (in first
> level only for a moment a I don't see level 2 to be safe).

agreed.

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

Oto BREZINA

unread,
Jun 14, 2013, 2:52:45 PM6/14/13
to d...@tortoisesvn.tigris.org
On 2013-06-14 18:45, Stefan Küng wrote:
> On 14.06.2013 17:48, Oto BREZINA wrote:
>
>>> haven't checked that part of the code. So you're saying that only
>>> leading tabs/spaces are converted but not if they're in the middle of
>>> the line?
>>> That's seems pretty useless to me, so I'm wondering why this was
>>> intentional?
>> Best suited style for us is indent by tabs allign by spaces (I'm not
>> going to explain).
> have a look at my tabspace tool:
> http://tools.tortoisesvn.net/tabspace.html
Too long to read :)

just question will it reformat VisualBasic notes correctly ?
>
> it works as I think you want it: using tabs but then spaces to align to
> mid-tab positions.
> And it does this not just at the beginning of lines but for all tabs/spaces.

I guess you missunderstand our formating rules.
tabs can be ONLY on line begining all other formatings have to be done
by spaces.
If you change tab size all formating is still working (ok 99% sometimes
sombody tries to format line end comment where indentation depth changes)
>
>>> I agree tri-state config was a little bit confusing but coverted 98% of
>>> cases if you understand it, now I get under 50%.
>>> I'm starting to wonder whether it's a good idea to even check and ask
>>> the user to fix these. It would be easier to just have buttons in the
>>> ribbon/toolbar to fix whatever needs to be fixed and have the user
>>> execute those manually if needed.
>>>
>>> That way the whole detection logic can be removed since I'm pretty sure
>>> it won't work in *all* situations, no matter how good we can make it.
>>>
>>> I have used a lot of IDE's and editors, and the ones that fix stuff
>>> automatically I stopped using because they mess things up without me
>>> noticing immediately (yes, sometimes I *need* a space at the end of a
>>> line). The ones that always ask to fix something I either configure to
>>> never do that again or I rarely use them anymore - too annoying.
>>> I use however those frequently that have the commands to fix stuff but I
>>> can do that manually - I know myself if something needs fixing.
>> As I wrote earlier some statuses (status bar - prefferably clickabel,
>> ribbon, toolbar) would be great, but I was affraid about proccessing
>> needed to keep them reasonably actual.
>> Just note: detection is to help when using commands from context menu too.
> detection is nice, yes. But it's not necessary: you can still have a
> command button enabled but when clicked doesn't change anything.
It is neccessary at least from my expierence - Nobody will ever hit fix
until they detect error, most people don't care about those error to
look for it, however if some button/status/whatever tell them there is
such error they can be teach to do so.
>
>> Ok lets remove auto fix stuff, commands are in context menu (in first
>> level only for a moment a I don't see level 2 to be safe).
> agreed.
you tabspace is level 2 i'll have closer look and CnP it
>
> Stefan
>

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

Oto BREZINA

unread,
Jun 16, 2013, 4:54:08 PM6/16/13
to d...@tortoisesvn.tigris.org
On 2013-06-14 20:52, Oto BREZINA wrote:
> On 2013-06-14 18:45, Stefan Küng wrote:
>> On 14.06.2013 17:48, Oto BREZINA wrote:
>>
>>>> haven't checked that part of the code. So you're saying that only
>>>> leading tabs/spaces are converted but not if they're in the middle of
>>>> the line?
>>>> That's seems pretty useless to me, so I'm wondering why this was
>>>> intentional?
>>> Best suited style for us is indent by tabs allign by spaces (I'm not
>>> going to explain).
>> have a look at my tabspace tool:
>> http://tools.tortoisesvn.net/tabspace.html
> Too long to read :)
>
> just question will it reformat VisualBasic notes correctly ?
I was trying to came with .cmd which tabspace can break I did not yet :)

I started to copying it into T-Merge, and found out that you have
bCStyle option to reduce conflict with other language files. This also
means that come with good implementation for T-Merge is not CnP, there
have to be good GUI too.
I will try to implement simplified version, but for me current
implementation is sufficient.

Any sugestions ?

------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3058222
Reply all
Reply to author
Forward
0 new messages