Opened 9 years ago

Closed 9 years ago

#5149 closed enhancement (fixed)

[Patch] Applying BGridLayout to Terminal preference

Reported by: mt Owned by: jackburton
Priority: normal Milestone: R1
Component: Applications/Terminal Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

Applying BGridLayout to BMenuFields in AppearPrefView.cpp.

Attachments (1)

terminal.patch (6.8 KB) - added by mt 9 years ago.
patch file

Download all attachments as: .zip

Change History (6)

Changed 9 years ago by mt

Attachment: terminal.patch added

patch file

comment:1 Changed 9 years ago by stippi

Patch looks good to me, thanks for the cleanup as well! Note that the -> operators would go on the next line. I usually break up these lines with an open parenthesis. So instead of

    fFont->Menu()->
        FindItem(PrefHandler::Default()->
        getString(PREF_HALF_FONT_FAMILY))->SetMarked(true);

I would do:

    fFont->Menu()->FindItem(PrefHandler::Default()->getString(
        PREF_HALF_FONT_FAMILY))->SetMarked(true);

(Your version does have it's appeal because of clarity, but the other version seems to be preferred among most devs.)

comment:2 Changed 9 years ago by stippi

BTW, I know the code was like that before and you just cleaned it up, but the defensive programmers version would be more like this:

    BMenuItem* item = fFont->Menu()->FindItem(
        PrefHandler::Default()->getString(PREF_HALF_FONT_FAMILY));
    if (item != NULL)
        item->SetMarked(true);

On the other hand, that would probably just hide an error elsewhere (mismatch of strings)... ;-)

comment:3 in reply to:  1 Changed 9 years ago by bonefish

Replying to stippi:

(Your version does have it's appeal because of clarity, but the other version seems to be preferred among most devs.)

Also very clear (even more so than the original version IMHO) and conforming to the style guide:

    fFont->Menu()->FindItem(PrefHandler::Default()->getString(
            PREF_HALF_FONT_FAMILY))
        ->SetMarked(true);

comment:4 Changed 9 years ago by jackburton

Status: newassigned

I'll apply this.

comment:5 Changed 9 years ago by jackburton

Resolution: fixed
Status: assignedclosed

Applied (with the change proposed by Ingo) in hrev34734. Thanks!

Note: See TracTickets for help on using tickets.