On Wed, 08 Feb 2012 12:03:58 -0000 mfilka@svn2.opensuse.org wrote:

Author: mfilka Date: Wed Feb 8 13:03:57 2012 New Revision: 67377

URL: http://svn.opensuse.org/viewcvs/yast?rev=67377&view=rev Log: Base server hostname combobox on existing values - bnc#547983

Hi, congratulate to your first patch. I review your commit and have just small notes:

snipped

Modified: trunk/nfs-client/src/ui.ycp URL: http://svn.opensuse.org/viewcvs/yast/trunk/nfs-client/src/ui.ycp?rev=67377&r1=67376&r2=67377&view=diff ============================================================================== --- trunk/nfs-client/src/ui.ycp (original) +++ trunk/nfs-client/src/ui.ycp Wed Feb 8 13:03:57 2012 @@ -58,6 +58,11 @@ list<string> hosts = nil;

/** + * List of already defined nfs mount points

Well, comment is not precise. If you want list of already defined nfs entries, it is exactly Nfs::nfs_entries. This variable store new state of nfs_entries ( so you have old one in Nfs and this variable store modified list by user and when user decide it is stored ).

+ */ + list< map > nfs_entries = Nfs::nfs_entries; + + /** * Let the user choose one of a list of items * @param title selectionbox title * @param items a list of items @@ -178,6 +183,7 @@ `Bottom (button) ); } + /** * Ask user for an entry. * @param fstab_ent $["spec": "file": "mntops":] or nil @@ -213,6 +219,21 @@ servers = [ proposed_server ]; }

+ // append already defined servers - bug #547983

good to have such comment, it really helps when WTF raised :)

+ foreach( map nfs_entry, nfs_entries, + { + term couple = SpecToServPath( nfs_entry[ "spec"]:""); + string known_server = ""; + + known_server = couple[0]:"";

It is confusing to have multiple lines without intention. simple: string known_server couple[0]:""; is enough in this case. Otherwise good commit. Josef

snipped -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org