review: add Video Memory text input to winecfg Graphics/Direct3D tab

[Vit Hrachovy]

Hi,
the attached patch adds new editable combobox input 'Video Memory size'
for Graphics/Direct3D tab of winecfg. 

I've tried to implement Frank's comments to English language only.
Please consider this patch as a proposal, so if it gets positive review,
I'll propagate the changes to all remaining language resources.

May I kindly ask someone for the patch review?
Regards
Vit

review: add Video Memory text input to winecfg Graphics/Direct3D tab

[Laurent Vromman]

case CBN_SELCHANGE: {
 		    SendMessage(GetParent(hDlg), PSM_CHANGED, 0, 0);
 		    switch (LOWORD(wParam)) {
-		    case IDC_D3D_VSHADER_MODE: on_d3d_vshader_mode_changed(hDlg); break;
-		    }
+                case IDC_D3D_VSHADER_MODE: on_d3d_vshader_mode_changed(hDlg); break;
+                case IDC_VIDEOMEMORY_SIZE_COMBO: set_from_videomemory_size_changed(hDlg); break;
+            }

I believe you should align your indentation method on the one the file already uses (ie tabs, not spaces).

+// vim:sw=4:expandtab

This has nothing to do with wine

Laurent


On Wed, 16 May 2007 15:03:52 +0200, Vit Hrachovy <vit.hrachovy(a)sandbox.cz> wrote:
> Hi,
> the attached patch adds new editable combobox input 'Video Memory size'
> for Graphics/Direct3D tab of winecfg.
>
> I've tried to implement Frank's comments to English language only.
> Please consider this patch as a proposal, so if it gets positive review,
> I'll propagate the changes to all remaining language resources.
>
> May I kindly ask someone for the patch review?
> Regards
> Vit

review: add Video Memory text input to winecfg Graphics/Direct3D tab

[Vit Hrachovy]

On Wed, May 16, 2007 at 03:18:22PM +0200, Laurent Vromman wrote:
> case CBN_SELCHANGE: {
>           SendMessage(GetParent(hDlg), PSM_CHANGED, 0, 0);
>           switch (LOWORD(wParam)) {
> -		    case IDC_D3D_VSHADER_MODE: on_d3d_vshader_mode_changed(hDlg); break;
> -		    }
> +                case IDC_D3D_VSHADER_MODE: on_d3d_vshader_mode_changed(hDlg); break;
> +                case IDC_VIDEOMEMORY_SIZE_COMBO: set_from_videomemory_size_changed(hDlg); break;
> +            }
>
> I believe you should align your indentation method on the one the file already uses (ie tabs, not spaces).
>
> +// vim:sw=4:expandtab
>
> This has nothing to do with wine
>
> Laurent

Hi Laurent
citing from: http://www.winehq.org/site/docs/winedev-guide/style-notes
	"Tabs are not forbidden but discouraged. A tab is defined as 8
	characters and the usual amount of indentation is 4 characters." 

and from: http://www.winehq.org/site/sending_patches
	"Don't mix tabs and spaces because it makes the diff output
	unreadable, use consistent indentation."

The actual file contents breaks both citations above, my code doesn't
conflict with any one of them.

For vim footer, I agree it shall not be present. If the only comment
regards formatting, I'm happy and I can fix the formatting as You wish.

Cheers
Vit

review: add Video Memory text input to winecfg Graphics/Direct3D tab

[Laurent Vromman]

It is not really as I wish. I have just noticed that there is two different kind of indentation in the mentioned switch loop, which looked strange on the moment.

I am really not a wine master, others will maybe fixed us.

Laurent


On Wed, 16 May 2007 15:59:06 +0200, Vit Hrachovy <vit.hrachovy(a)sandbox.cz> wrote:
> On Wed, May 16, 2007 at 03:18:22PM +0200, Laurent Vromman wrote:
> > case CBN_SELCHANGE: {
> >           SendMessage(GetParent(hDlg), PSM_CHANGED, 0, 0);
> >           switch (LOWORD(wParam)) {
> > -		    case IDC_D3D_VSHADER_MODE: on_d3d_vshader_mode_changed(hDlg);
>
> break;
> > -		    }
> > +                case IDC_D3D_VSHADER_MODE:
> > on_d3d_vshader_mode_changed(hDlg); break;
> > +                case IDC_VIDEOMEMORY_SIZE_COMBO:
> > set_from_videomemory_size_changed(hDlg); break;
> > +            }
> >
> > I believe you should align your indentation method on the one the file
> > already uses (ie tabs, not spaces).
> >
> > +// vim:sw=4:expandtab
> >
> > This has nothing to do with wine
> >
> > Laurent
>
> Hi Laurent
> citing from: http://www.winehq.org/site/docs/winedev-guide/style-notes
>   "Tabs are not forbidden but discouraged. A tab is defined as 8
>   characters and the usual amount of indentation is 4 characters."
>
> and from: http://www.winehq.org/site/sending_patches
>   "Don't mix tabs and spaces because it makes the diff output
>   unreadable, use consistent indentation."
>
> The actual file contents breaks both citations above, my code doesn't
> conflict with any one of them.
>
> For vim footer, I agree it shall not be present. If the only comment
> regards formatting, I'm happy and I can fix the formatting as You wish.
>
> Cheers
> Vit

[Frank Richter]

I would make the combobox editable (plain CBS_DROPDOWN), just add all
"preset" memory sizes, and WM_SETTEXT the value read from the registry.

-f.r.

[Frank Richter]

On 16.05.2007 18:18, Frank Richter wrote:
> I would make the combobox editable (plain CBS_DROPDOWN),

Er, you do that already. Pays off to read...
> just add all
> "preset" memory sizes, and WM_SETTEXT the value read from the registry.

Might be code-wise a bit simpler than your GETCURSEL approach, but
otherswise not much different I think.

-f.r.

review: add Video Memory text input to winecfg Graphics/Direct3D tab

[Vit Hrachovy]

On Wed, May 16, 2007 at 06:41:33PM +0200, Frank Richter wrote:
> > just add all
> > "preset" memory sizes, and WM_SETTEXT the value read from the registry.
>
> Might be code-wise a bit simpler than your GETCURSEL approach, but
> otherswise not much different I think.
>
> -f.r.

Hi Frank,
I've simplified the code according Your review, see the attached patch.
Thanks for help.
Vit