[PATCH] Ticket #473: Add option of saving User ID in xml

[Venkatesh Shukla]

On Thu, Apr 10, 2014 at 10:35 PM, Dirk Hohndel <dirk at hohndel.org> wrote:

>
> Hi Venkatesh,
>
> sorry this took longer than usual - things have been crazy busy for me.
> Sadly none of the other developers provided review, either (cough,
> cough)...
>
>
> > Added feature of saving subsurface userid in local files (xml as well
> > as the git repo) as per ticket #473
> >
> >
> > For git repo, userid is stored in the 00-Subsurface file present in
> > the repo. For xml files, it is stored under tag userid.
>
> This seems reasonable.
>
> > Modified preference and webservice dialog to add these options.
> > Import, open and save as dialog now show git repo pointer files as
> > well. For ex /home/subsurface/linus[dives]
>
> I don't think your patch does what we would want there - adding "*]" as
> pattern assumes that this is how those files look in a file browser.
> But of course they don't as this is simply magic syntax. This part of
> the code (how does a non-expert user figure out saving to a git
> repository) is far from done... right now you really need to manually
> enter the magic syntax.
> (and even if this were correct, I still wouldn't want it mixed in with
> this patch as it has nothing to do with saving the userid)
>

I agree with you. This system needs to be developed further and polished. I
found it difficult to manually enter the name each and every time. Hence, I
made those changes. I deleted those in patch attached.


>
> > Complete patch is attached.
>
> Some comments on the patch:
>
> Why is the userid not part of the settings section in the XML file? That
> would correspond to the 00-Subsurface file in git :-)
>

Yes, of course. Made changes regarding this.


>
> +static void parse_settings_userid(char *line, struct membuffer *str,
> void *_unused)
> +{
> +       char *uid = line;
> +       if (uid)
> +       {
> +               set_save_userid_local(true);
> +               set_userid(uid);
> +       }
> +}
>
> Why do you have that local variable "uid"? Wouldn't it be much easier to
> just use "line"?
>
>
> +static void save_userid(void *_b)
> +{
> +       struct membuffer *b = _b;
> +       if (save_userid_local) {
> +               put_format(b, "userid %30s", userid);
> +               printf("saving userid as save_userid_local is true :
> %s\n", userid);
> +       } else {
> +               printf("not saving userid as save_userid_local is
> false\n");
> +       }
> +}
> +
>
> this looks like some debug output snuck into your final patch...
>

Extremely sorry for this. Would not happen again.


>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +#define MAX_USERID_SIZE 32
> +short save_userid_local = false;
> +char *userid = NULL;
> +void set_save_userid_local(short value)
> +{
> +       QSettings s;
> +       s.setValue("save_uid_local", value);
> +       save_userid_local = value;
> +}
> +
> +void set_userid(char *rUserId)
> +{
> +       userid = (char *) malloc(MAX_USERID_SIZE);
> +       if (userid && rUserId)
> +               strcpy(userid, rUserId);
> +}
> +
> +#ifdef __cplusplus
> +}
> +#endif
>
> Why do you have the __cplusplus macro in the .cpp file? You don't need
> to do that, only wrap the prototypes in the .h file.


> OK, that's what I found so far - maybe others see more issues (hint,
> hint) :-)
>
> /D
>
>
I made the required changes. The patch is attached.

As you have hinted, there might be more problems in this patch. Please do
mention them so that I can improve both the patch and myself.
Eagerly waiting for your feedbacks.

-- 

*Venkatesh Shukla *
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20140411/b90a65c1/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Adds-option-of-saving-userid-in-local-files.patch
Type: text/x-patch
Size: 12835 bytes
Desc: not available
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20140411/b90a65c1/attachment-0001.bin>