Login | Register
My pages Projects Community openCollabNet

Discussions > dev > Re: [T-Merge patch] syncing code for views

Project highlights: :. Download .: :. Support .: :. FAQ .: :. Translations .: :. Donate .: :. Report Bug .:

tortoisesvn
Discussion topic

Hide all messages in topic

All messages in topic

Re: [T-Merge patch] syncing code for views

Author steveking
Full name Stefan Küng
Date 2011-05-12 12:52:58 PDT
Message On 11.05.2011 22:54, Oto BREZINA wrote:

> I tried this and some other aproach as well. In best case I was able to
> get some handle, but it wont display icon on popup menu. However when I
> use some bitmap/icon from resource it works.

Got it working. But using the already loaded toolbar instead of loading
it from scratch.

Stefan

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

Re: [T-Merge patch] syncing code for views

Author otik
Full name Oto BREZINA
Date 2011-05-11 13:54:50 PDT
Message On 2011-04-17 11:46, Stefan Küng wrote:
> On 17.04.2011 09:30, Oto BREZINA wrote:
>> On 2011-04-05 20:03, Stefan Küng wrote:
>>>> 2. Is there plan to support icons there? let say, same like in main
>>>> menu? not worth much but nice :)
>>> Good idea.
>> I look at code it seems be possible without owner draw to keep string.
>> (ref: http://www.codeguru.​com/forum/archive/in​dex.php/t-101074.htm​l)
>> However I don't know how to extract one bitmap from toolbar ...
>> Do we need set new resource?
> To get the icon for a specific command from the toolbar, something like
> this might work:
>
> HICON GetIconForCommand() {
> CToolbar bar;
> bar.Create(m_hWnd);
> if (bar.LoadToolBar(IDR​_MAINFRAME))
> {
> int offset = bar.CommandToIndex(cmdId);
> CImageList imgList;
> imgList.Attach(SendM​essage(bar.GetSafeHw​nd(), TB_GETIMAGELIST, 0, 0));
> HICON h=imgList.ExtractIcon(offset);
> imgList.Detach();
> return h;
> }
> return 0;
> }
> If a handle is returned, use that for the AppendMenuIcon() method.
> Otherwise (if 0 is returned) you can use a custom icon.
I tried this and some other aproach as well. In best case I was able to
get some handle, but it wont display icon on popup menu. However when I
use some bitmap/icon from resource it works.
> Stefan
>

--
Oto BREZINA, Printflow s.r.o., EU
+421 903 653470 skype: ot_ik_

3D"My <3D%22skype:tomas​.blaho?call%22>​<3D%22skype:tomas.b​laho?call%22>
Attachments

Re: [T-Merge patch] syncing code for views

Author steveking
Full name Stefan Küng
Date 2011-04-17 02:46:45 PDT
Message On 17.04.2011 09:30, Oto BREZINA wrote:
> On 2011-04-05 20:03, Stefan Küng wrote:
>>
>>> 2. Is there plan to support icons there? let say, same like in main
>>> menu? not worth much but nice :)
>> Good idea.
> I look at code it seems be possible without owner draw to keep string.
> (ref: http://www.codeguru.​com/forum/archive/in​dex.php/t-101074.htm​l)
> However I don't know how to extract one bitmap from toolbar ...
> Do we need set new resource?

To get the icon for a specific command from the toolbar, something like
this might work:

HICON GetIconForCommand() {
CToolbar bar;
bar.Create(m_hWnd);
if (bar.LoadToolBar(IDR​_MAINFRAME))
{
   int offset = bar.CommandToIndex(cmdId);
   CImageList imgList;
   imgList.Attach(SendM​essage(bar.GetSafeHw​nd(), TB_GETIMAGELIST, 0, 0));
   HICON h=imgList.ExtractIcon(offset);
   imgList.Detach();
   return h;
}
return 0;
}

If a handle is returned, use that for the AppendMenuIcon() method.
Otherwise (if 0 is returned) you can use a custom icon.

Stefan

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

Re: [T-Merge patch] syncing code for views

Author otik
Full name Oto BREZINA
Date 2011-04-17 00:31:07 PDT
Message On 2011-04-05 20:03, Stefan Küng wrote:
>
>> 2. Is there plan to support icons there? let say, same like in main
>> menu? not worth much but nice :)
> Good idea.
I look at code it seems be possible without owner draw to keep string.
(ref: http://www.codeguru.​com/forum/archive/in​dex.php/t-101074.htm​l)
However I don't know how to extract one bitmap from toolbar ...
Do we need set new resource?
--
Oto BREZINA, Printflow s.r.o., EU

Re: [T-Merge patch] syncing code for views

Author otik
Full name Oto BREZINA
Date 2011-04-10 12:44:39 PDT
Message On 2011-04-10 21:34, Stefan Küng wrote:
> On 10.04.2011 21:21, Oto BREZINA wrote:
> But does the checkout page show you the https url? If not, then maybe
> you're not logged in with the right user and htcotik is not the one
> you're using?
>
> Also maybe clearing the auth cache in the TSVN settings dialog is worth
> a try.
I did it. Googlecode uses different password than my account use...
TNX.
> Stefan
--
Oto BREZINA, Printflow s.r.o., EU

Re: [T-Merge patch] syncing code for views

Author steveking
Full name Stefan Küng
Date 2011-04-10 12:35:07 PDT
Message On 10.04.2011 21:21, Oto BREZINA wrote:
> On 2011-04-10 21:14, Stefan Küng wrote:
>> On 10.04.2011 21:08, Oto BREZINA wrote:
>>> On 2011-04-10 11:15, Stefan Küng wrote:
>>> I tried short and long login, with my new created password, but can't
>>> commit. I get authentification error. Any hint ?
>> If you see the text "# Project members authenticate over HTTPS to allow
>> committing changes." on the source checkout page
>> (http://code.google.c​om/p/tortoisesvn/sou​rce/checkout), then you're
>> logged in correctly.
>> But as the page tells you, you have to check out the working copy with
>> the https url, not the http url that non-members have to use.
>> You can not commit to the http url, only to the https one.
> I did relocate to https succesfully.
>
> Chyba: Commit failed (details follow):
> Chyba: MKACTIVITY of '/svn/!svn/act/e07d4​be3-3419-3742-8579-0​3ccff9b621c':
> Chyba: authorization failed: Could not authenticate to server: rejected
> Basic
> Chyba: challenge (https://tortoisesvn.​googlecode.com)

But does the checkout page show you the https url? If not, then maybe
you're not logged in with the right user and htcotik is not the one
you're using?

Also maybe clearing the auth cache in the TSVN settings dialog is worth
a try.

Stefan


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

Re: [T-Merge patch] syncing code for views

Author otik
Full name Oto BREZINA
Date 2011-04-10 12:22:04 PDT
Message On 2011-04-10 21:14, Stefan Küng wrote:
> On 10.04.2011 21:08, Oto BREZINA wrote:
>> On 2011-04-10 11:15, Stefan Küng wrote:
>> I tried short and long login, with my new created password, but can't
>> commit. I get authentification error. Any hint ?
> If you see the text "# Project members authenticate over HTTPS to allow
> committing changes." on the source checkout page
> (http://code.google.c​om/p/tortoisesvn/sou​rce/checkout), then you're
> logged in correctly.
> But as the page tells you, you have to check out the working copy with
> the https url, not the http url that non-members have to use.
> You can not commit to the http url, only to the https one.
I did relocate to https succesfully.

Chyba: Commit failed (details follow):
Chyba: MKACTIVITY of '/svn/!svn/act/e07d4​be3-3419-3742-8579-0​3ccff9b621c':
Chyba: authorization failed: Could not authenticate to server: rejected
Basic
Chyba: challenge (https://tortoisesvn.​googlecode.com)

> Stefan

--
Oto BREZINA, Printflow s.r.o., EU

Re: [T-Merge patch] syncing code for views

Author steveking
Full name Stefan Küng
Date 2011-04-10 12:14:39 PDT
Message On 10.04.2011 21:08, Oto BREZINA wrote:
> On 2011-04-10 11:15, Stefan Küng wrote:
>> On 09.04.2011 15:49, Oto BREZINA wrote:
>>
>> > htcotik - at least I think .. :) it is only for TSVN (and my phone) :D
>> > I will not commit anything but poChecker (unless you send me free VC2010
>> > copy :D )
>>
>> I've added htcotik as a committer. You should now be able to commit to
>> the repository.
> I tried short and long login, with my new created password, but can't
> commit. I get authentification error. Any hint ?

If you see the text "# Project members authenticate over HTTPS to allow
committing changes." on the source checkout page
(http://code.google.c​om/p/tortoisesvn/sou​rce/checkout), then you're
logged in correctly.
But as the page tells you, you have to check out the working copy with
the https url, not the http url that non-members have to use.
You can not commit to the http url, only to the https one.

Stefan

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

Re: [T-Merge patch] syncing code for views

Author otik
Full name Oto BREZINA
Date 2011-04-10 12:08:55 PDT
Message On 2011-04-10 11:15, Stefan Küng wrote:
> On 09.04.2011 15:49, Oto BREZINA wrote:
>
> > htcotik - at least I think .. :) it is only for TSVN (and my phone) :D
> > I will not commit anything but poChecker (unless you send me free VC2010
> > copy :D )
>
> I've added htcotik as a committer. You should now be able to commit to
> the repository.
I tried short and long login, with my new created password, but can't
commit. I get authentification error. Any hint ?
> Stefan
>

--
Oto BREZINA, Printflow s.r.o., EU

Re: [T-Merge patch] syncing code for views

Author steveking
Full name Stefan Küng
Date 2011-04-10 02:15:30 PDT
Message On 09.04.2011 15:49, Oto BREZINA wrote:

 > htcotik - at least I think .. :) it is only for TSVN (and my phone) :D
 > I will not commit anything but poChecker (unless you send me free VC2010
 > copy :D )

I've added htcotik as a committer. You should now be able to commit to
the repository.

Stefan

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

Re: [T-Merge patch] syncing code for views

Author steveking
Full name Stefan Küng
Date 2011-04-10 02:14:27 PDT
Message On 09.04.2011 21:54, Oto BREZINA wrote:

 >> It's not a performance issue but a code size issue. The switch-case
 >> statement is converted into a jump table, and if the values are far
 >> apart, that jump table gets really big. And we have to trust the
 >> optimizer to decide in such cases that a jump table isn't the right
 >> thing to do but better convert the switch-case to if-else statements.

 > I would say numbers are from big range right now:
 > ID_EDIT_COPY and ID_EDIT_PASTE are 0xE122 and 0xE125 respectively.
 > http://msdn.microsof​t.com/en-us/library/​dd941781%28v=vs.85%2​9.aspx - may
 > be not a relevant source I have seen lot of other values, but this one
 > is MSDN.
 >
 > But I'm looking for other solution..

Those values are not fixed but are assigned automatically by the
resource editor. And I don't like to have to check the assigned values
every time I add a new string.

Stefan


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

Re: [T-Merge patch] syncing code for views

Author otik
Full name Oto BREZINA
Date 2011-04-09 12:54:25 PDT
Message On 2011-04-06 18:25, Stefan Küng wrote:
> On 05.04.2011 23:09, Oto BREZINA wrote:
>>
>>>> popup.AppendMenu(
>>>> UINT nFlags,
>>>> UINT_PTR nIDNewItem = 0,
>>>> LPCTSTR lpszNewItem = NULL )
>>>>
>>>> 1. Is there any real gain to do not use nIDNewItem same value as IDS ?
>>> That depends on whether we trust the optimizations of the compiler :)
>>> If we use the IDS values, then the switch() could take up a lot of space
>>> since those values are not continuous or even in the same value range.
>>> Sure, we could edit the resource.h file manually to make sure those IDS
>>> values are as we would like them, but I don't like messing with
>>> generated files.
>> we are talking about user action response - called rare, with up to ten
>> values. No big deal, but I will keep in mind ...
> It's not a performance issue but a code size issue. The switch-case
> statement is converted into a jump table, and if the values are far
> apart, that jump table gets really big. And we have to trust the
> optimizer to decide in such cases that a jump table isn't the right
> thing to do but better convert the switch-case to if-else statements.
I would say numbers are from big range right now:
ID_EDIT_COPY and ID_EDIT_PASTE are 0xE122 and 0xE125 respectively.
http://msdn.microsof​t.com/en-us/library/​dd941781%28v=vs.85%2​9.aspx - may
be not a relevant source I have seen lot of other values, but this one
is MSDN.

But I'm looking for other solution..
> Stefan
>

--
Oto BREZINA, Printflow s.r.o., EU

Re: [T-Merge patch] syncing code for views

Author otik
Full name Oto BREZINA
Date 2011-04-09 06:50:02 PDT
Message Attached part5
Reduce code duplication in T-Merge views

On 2011-04-09 09:17, Stefan Küng wrote:
> On 08.04.2011 22:09, Oto BREZINA wrote:
>> On 2011-04-08 21:00, Stefan Küng wrote:
>> I thought to ask you for it, but only for poChecker - code changes
>> become quite big.
> Just send me your google account name and I'll set you up.
> But you have to make sure that every commit compiles properly - bugs can
> be fixed later, but if the compile fails, the nightly builds also fail
> and that's not what I want.
htcotik - at least I think .. :) it is only for TSVN (and my phone) :D
I will not commit anything but poChecker (unless you send me free VC2010
copy :D )
>> All (most of) that function can be moved to parent class, but it become
>> a big mess, so I like to keep their place just remove duplicates if
>> possible.
> I thought the same when I had to fix this. The bottom view class is
> really the best place for this.
In this patch I still kept location as it. Duplicates has been resolved
to pulling class (as you can see).

>> This also lead me to question:
>> CLeftView::UseFile handle DIFFSTATE_CONFLICTEMPTY as special state to be
>> resolved into DIFFSTATE_CONFLICTRE​SOLVEDEMPTY, unlike
>> CRightView::UseFile, CBottomView::UseTheirTextBlock,
>> CBottomView::UseMyTextBlock.
> Fixed CRightView::UseFile in r21132, but in CBottomView those states are
> already handled properly?
Even my comments are buggy, CBottomView method should be on left side in
that sentence ...
--
Oto BREZINA, Printflow s.r.o., EU
Attachments

Re: [T-Merge patch] syncing code for views

Author steveking
Full name Stefan Küng
Date 2011-04-09 00:17:09 PDT
Message On 08.04.2011 22:09, Oto BREZINA wrote:
> On 2011-04-08 21:00, Stefan Küng wrote:
>> On 08.04.2011 00:23, Oto BREZINA wrote:
>>> Part4:
>>> Introduce and use ResetUndoStep, SaveUndoStep and m_Allstate to handle
>>> undo step build time
>>> BaseView: Use HasSelection() where applicable
>> Some comments:
>> * doesn't compile
> I don't have VC, so that was bit expected. I thought to buy it, but it
> is too expensive to use that occasionally. All I have is T-Merge :)
>> * style is wrong ( "{" on same line instead of the next line, ...)
> we use that one, so sorry, I know you have only few requerement for
> patch and this is one of them :)
> reference:
> http://tortoisesvn.t​igris.org/ds/viewMes​sage.do?dsForumId=75​7&dsMessageId=25​87752
>
>> Just when I thought you're ready for commit access :/
> I thought to ask you for it, but only for poChecker - code changes
> become quite big.

Just send me your google account name and I'll set you up.
But you have to make sure that every commit compiles properly - bugs can
be fixed later, but if the compile fails, the nightly builds also fail
and that's not what I want.

> I like to use pair programing especially with code "owner"/main maintainer.
>> I fixed, modified it slightly and committed it in r21127.
> I hope I do more work then make(prepare for you) :)
>
> I missed that dynamic_cast is needed, do you know about its overhead?

Not much overhead. And since those functions are only executed once on
users request, the "overhead" for showing the context menu and the
actual function is much much bigger.

> All (most of) that function can be moved to parent class, but it become
> a big mess, so I like to keep their place just remove duplicates if
> possible.

I thought the same when I had to fix this. The bottom view class is
really the best place for this.

>
> This also lead me to question:
> CLeftView::UseFile handle DIFFSTATE_CONFLICTEMPTY as special state to be
> resolved into DIFFSTATE_CONFLICTRE​SOLVEDEMPTY, unlike
> CRightView::UseFile, CBottomView::UseTheirTextBlock,
> CBottomView::UseMyTextBlock.

Fixed CRightView::UseFile in r21132, but in CBottomView those states are
already handled properly?

> Also in two-panel view are BuildAllScreen2ViewVector()
> RecalcAllVertScrollBars() calls missing.

Fixed in r21133.

Stefan

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

Re: [T-Merge patch] syncing code for views

Author otik
Full name Oto BREZINA
Date 2011-04-08 13:10:05 PDT
Message On 2011-04-08 21:00, Stefan Küng wrote:
> On 08.04.2011 00:23, Oto BREZINA wrote:
>> Part4:
>> Introduce and use ResetUndoStep, SaveUndoStep and m_Allstate to handle
>> undo step build time
>> BaseView: Use HasSelection() where applicable
> Some comments:
> * doesn't compile
I don't have VC, so that was bit expected. I thought to buy it, but it
is too expensive to use that occasionally. All I have is T-Merge :)
> * style is wrong ( "{" on same line instead of the next line, ...)
we use that one, so sorry, I know you have only few requerement for
patch and this is one of them :)
reference:
http://tortoisesvn.t​igris.org/ds/viewMes​sage.do?dsForumId=75​7&dsMessageId=25​87752

> Just when I thought you're ready for commit access :/
I thought to ask you for it, but only for poChecker - code changes
become quite big.
I like to use pair programing especially with code "owner"/main maintainer.
> I fixed, modified it slightly and committed it in r21127.
I hope I do more work then make(prepare for you) :)

I missed that dynamic_cast is needed, do you know about its overhead?
All (most of) that function can be moved to parent class, but it become
a big mess, so I like to keep their place just remove duplicates if
possible.

This also lead me to question:
CLeftView::UseFile handle DIFFSTATE_CONFLICTEMPTY as special state to be
resolved into DIFFSTATE_CONFLICTRE​SOLVEDEMPTY, unlike
CRightView::UseFile, CBottomView::UseTheirTextBlock,
CBottomView::UseMyTextBlock.
Also in two-panel view are BuildAllScreen2ViewVector()
RecalcAllVertScrollBars() calls missing.

> Stefan
>

--
Oto BREZINA, Printflow s.r.o., EU

Re: [T-Merge patch] syncing code for views

Author steveking
Full name Stefan Küng
Date 2011-04-08 12:01:02 PDT
Message On 08.04.2011 00:23, Oto BREZINA wrote:
> Part4:
> Introduce and use ResetUndoStep, SaveUndoStep and m_Allstate to handle
> undo step build time
> BaseView: Use HasSelection() where applicable

Some comments:
* doesn't compile
* style is wrong ( "{" on same line instead of the next line, ...)

Just when I thought you're ready for commit access :/

I fixed, modified it slightly and committed it in r21127.

Stefan

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

Re: [T-Merge patch] syncing code for views

Author otik
Full name Oto BREZINA
Date 2011-04-07 15:23:20 PDT
Message Part4:
Introduce and use ResetUndoStep, SaveUndoStep and m_Allstate to handle
undo step build time
BaseView: Use HasSelection() where applicable

On 2011-04-07 22:30, Stefan Küng wrote:
> On 07.04.2011 22:20, Oto BREZINA wrote:
>>
>> On 2011-04-07 21:42, Stefan Küng wrote:
>> You introduce new bug by copy-paste ... nevermind Bottom::UseMyTextBlock
>> will be reused for that part.
> Ups! Fixed in r21126.
I almost used buggy method instead of correct.
> Stefan
>

--
Oto BREZINA, Printflow s.r.o., EU
Attachments

Re: [T-Merge patch] syncing code for views

Author steveking
Full name Stefan Küng
Date 2011-04-07 13:30:40 PDT
Message On 07.04.2011 22:20, Oto BREZINA wrote:
>
>
> On 2011-04-07 21:42, Stefan Küng wrote:
>> On 07.04.2011 21:03, Oto BREZINA wrote:
>>> It is not in: CBottomView::UseMyTextBlock
>>> While it is in: CBottomView::UseTheirTextBlock, CLeftView::UseBlocka and
>>> CRightView::UseBlock(
>> Ups, you're right. That's a bug.
>> Fixed in r21121.
> You introduce new bug by copy-paste ... nevermind Bottom::UseMyTextBlock
> will be reused for that part.

Ups! Fixed in r21126.

Stefan

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

Re: [T-Merge patch] syncing code for views

Author otik
Full name Oto BREZINA
Date 2011-04-07 13:20:49 PDT
Message On 2011-04-07 21:42, Stefan Küng wrote:
> On 07.04.2011 21:03, Oto BREZINA wrote:
>> It is not in: CBottomView::UseMyTextBlock
>> While it is in: CBottomView::UseTheirTextBlock, CLeftView::UseBlocka and
>> CRightView::UseBlock(
> Ups, you're right. That's a bug.
> Fixed in r21121.
You introduce new bug by copy-paste ... nevermind Bottom::UseMyTextBlock
will be reused for that part.
> Stefan
>

--
Oto BREZINA, Printflow s.r.o., EU
+421 903 653470 skype: ot_ik_

3D"My <3D%22skype:tomas​.blaho?call%22>​<3D%22skype:tomas.b​laho?call%22>
Attachments

Re: [T-Merge patch] syncing code for views

Author steveking
Full name Stefan Küng
Date 2011-04-07 12:42:35 PDT
Message On 07.04.2011 21:03, Oto BREZINA wrote:
> On 2011-04-07 19:16, Stefan Küng wrote:
>> On 07.04.2011 00:11, Oto BREZINA wrote:
>>> sometime if (m_pwndBottom->Is​ViewLineConflicted(v​iewLine)) is used while on other places not.
>> It's used whenever the bottom view is visible. Or do you see another place?
>
> It is not in: CBottomView::UseMyTextBlock
> While it is in: CBottomView::UseTheirTextBlock, CLeftView::UseBlocka and
> CRightView::UseBlock(

Ups, you're right. That's a bug.
Fixed in r21121.

> btw: m_nSelBlockStart and m_nSelBlockEnd synced for all views, or can be
> different?

It should not be different.

> I think about add 'static allviewstate m_allviewstate' into BaseView.
> Then usage would be as follow:
> ClearUndoStep();
> ... call methods copiing data, modififyng state ...
> AddUndoStep(caret); // will not add if no data to add
> This removes creating, and pushing states ...
> btw

Looks like a good idea.

Stefan

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

Re: [T-Merge patch] syncing code for views

Author otik
Full name Oto BREZINA
Date 2011-04-07 12:03:24 PDT
Message On 2011-04-07 19:16, Stefan Küng wrote:
> On 07.04.2011 00:11, Oto BREZINA wrote:
>> sometime if (m_pwndBottom->Is​ViewLineConflicted(v​iewLine)) is used while on other places not.
> It's used whenever the bottom view is visible. Or do you see another place?

It is not in: CBottomView::UseMyTextBlock
While it is in: CBottomView::UseTheirTextBlock, CLeftView::UseBlocka and
CRightView::UseBlock(



btw: m_nSelBlockStart and m_nSelBlockEnd synced for all views, or can be
different?


I think about add 'static allviewstate m_allviewstate' into BaseView.
Then usage would be as follow:
ClearUndoStep();
... call methods copiing data, modififyng state ...
AddUndoStep(caret); // will not add if no data to add
This removes creating, and pushing states ...
btw

> Stefan
>

--
Oto BREZINA, Printflow s.r.o., EU

Re: [T-Merge patch] syncing code for views

Author steveking
Full name Stefan Küng
Date 2011-04-07 10:16:41 PDT
Message On 07.04.2011 00:11, Oto BREZINA wrote:
> Part3:
>
> prepend object name to simplify comparison/extraction common code
> use HasSelection() where aplicable
> Bottom: reorder viewstate triplet to order as used in LeftView and RightView
> Left: rename temporary variable from state2 to state
> Right: move SetModified out of for cycle

Committed in r21118.

> Note: by comparing LeftView, RightView and BottomView you can get nice overview about common part and differences
>
> In LeftView there are two BuildAllScreen2ViewVector(); RecalcAllVertScrollBars();, while in RightView on same place there are not those calls. Also not sure if they may be outside if (e.g. m_pwndLocator->Do​cumentUpdated(); is affected).

Fixed in r21119.

> sometime if (m_pwndBottom->Is​ViewLineConflicted(v​iewLine)) is used while on other places not.

It's used whenever the bottom view is visible. Or do you see another place?

Stefan

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

Re: [T-Merge patch] syncing code for views

Author otik
Full name Oto BREZINA
Date 2011-04-06 15:12:10 PDT
Message Part3:

prepend object name to simplify comparison/extraction common code
use HasSelection() where aplicable
Bottom: reorder viewstate triplet to order as used in LeftView and RightView
Left: rename temporary variable from state2 to state
Right: move SetModified out of for cycle


Note: by comparing LeftView, RightView and BottomView you can get nice overview about common part and differences

In LeftView there are two BuildAllScreen2ViewVector(); RecalcAllVertScrollBars();, while in RightView on same place there are not those calls. Also not sure if they may be outside if (e.g. m_pwndLocator->Do​cumentUpdated(); is affected).

sometime if (m_pwndBottom->Is​ViewLineConflicted(v​iewLine)) is used while on other places not.


--
Oto BREZINA, Printflow s.r.o., EU
Attachments

Re: [T-Merge patch] syncing code for views

Author steveking
Full name Stefan Küng
Date 2011-04-06 09:26:00 PDT
Message On 05.04.2011 23:09, Oto BREZINA wrote:
> On 2011-04-05 20:03, Stefan Küng wrote:
>> On 04.04.2011 22:57, Oto BREZINA wrote:
>>> Theme of patch: syncing code of BottomView.cpp, LeftView.cpp,
>>> RightView.cpp for later refactor common part to (BaseView.cpp)
> (same theme) part 2 attached : LeftView: extracting code from switch
> into functions; RightView add spaces to match LeftView formating;
> BaseView remove commented block defining WM_MOUSEHWHEEL fo pre 600 wins
>>> popup.AppendMenu(
>>> UINT nFlags,
>>> UINT_PTR nIDNewItem = 0,
>>> LPCTSTR lpszNewItem = NULL )
>>>
>>> 1. Is there any real gain to do not use nIDNewItem same value as IDS ?
>> That depends on whether we trust the optimizations of the compiler :)
>> If we use the IDS values, then the switch() could take up a lot of space
>> since those values are not continuous or even in the same value range.
>> Sure, we could edit the resource.h file manually to make sure those IDS
>> values are as we would like them, but I don't like messing with
>> generated files.
> we are talking about user action response - called rare, with up to ten
> values. No big deal, but I will keep in mind ...

It's not a performance issue but a code size issue. The switch-case
statement is converted into a jump table, and if the values are far
apart, that jump table gets really big. And we have to trust the
optimizer to decide in such cases that a jump table isn't the right
thing to do but better convert the switch-case to if-else statements.

Stefan

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

Re: [T-Merge patch] syncing code for views

Author otik
Full name Oto BREZINA
Date 2011-04-05 14:10:14 PDT
Message On 2011-04-05 20:03, Stefan Küng wrote:
> On 04.04.2011 22:57, Oto BREZINA wrote:
>> Theme of patch: syncing code of BottomView.cpp, LeftView.cpp,
>> RightView.cpp for later refactor common part to (BaseView.cpp)
(same theme) part 2 attached : LeftView: extracting code from switch
into functions; RightView add spaces to match LeftView formating;
BaseView remove commented block defining WM_MOUSEHWHEEL fo pre 600 wins
>> popup.AppendMenu(
>> UINT nFlags,
>> UINT_PTR nIDNewItem = 0,
>> LPCTSTR lpszNewItem = NULL )
>>
>> 1. Is there any real gain to do not use nIDNewItem same value as IDS ?
> That depends on whether we trust the optimizations of the compiler :)
> If we use the IDS values, then the switch() could take up a lot of space
> since those values are not continuous or even in the same value range.
> Sure, we could edit the resource.h file manually to make sure those IDS
> values are as we would like them, but I don't like messing with
> generated files.
we are talking about user action response - called rare, with up to ten
values. No big deal, but I will keep in mind ...

> Stefan
>

--
Oto BREZINA, Printflow s.r.o., EU
Attachments
Page: of 2 « Previous | Next »
Messages per page: