I like it, I'll update it to BVariantSetValue.
On Thu, Dec 4, 2014 at 4:08 PM, Rob Mensching <r...@firegiant.com> wrote:
> A’ight, cool. I’m with you on this. There is only one thing. I’m not big
> on the name BVariantSetVariant(). I left this comment on the PR:
>
>
>
> I don't really like the name `BVariantSetVariant`. I get what it is saying
> but it sounds funny to me. It also doesn't really say that it isn't setting
> the encryption state (I view the encryption state as part of the
> *variant*). What about `BVariantSetValue`? That scopes it more to just the
> value part of the variant... thoughts?
>
>
>
> _______________________________________________________________
>
> FireGiant | Dedicated support for the WiX toolset |
> http://www.firegiant.com/
>
>
>
> *From:* Sean Hall [mailto:r.sean.h...@gmail.com]
> *Sent:* Thursday, November 27, 2014 7:59 PM
> *To:* WiX toolset developer mailing list
> *Subject:* Re: [WiX-devs] Just opened bug 4609 against v3.9
>
>
>
> When I look at the current usage of BVariantCopy, I see two different uses.
>
>
>
> The first usage is making a local copy of the variant, this usage matches
> the name and what I had in mind when I was modifying it. Here we always
> want the local copy's (pTarget) encryption state to match the original
> (pSource).
>
>
>
> The second usage is being lazy, because what it really wants to do is set
> the target's value to the source's value, making BVariantCopy figure out
> which type the source is. This is why it makes sense to me to name this
> like the other BVariantSet* methods. From there it makes sense to me that
> just like the other BVariantSet* methods, the encryption state shouldn't
> change.
>
>
>
> After typing that out, I now see why you were thinking about
> BVariantAssign. VariablesParseFromXml calls BVariantCopy with my second
> usage. But it immediately calls BVariantSetEncryption, so it's actually a
> third kind of usage - setting the target's value and encryption state at
> the same time. On the other hand, I have a feeling that after optimizing
> that code it will not be lazy and will call the appropriate BVariantSet*
> method directly, with a BVariantSetEncryption call at the end.
>
>
>
> This is my long winded answer to Rob's comment on my pull request
> <https://github.com/wixtoolset/wix3/pull/181>: the scenario where the
> source is encrypted but the target is not encrypted can't happen in the
> current code. And I don't see a valid use case for it either. More
> comments are always good, though :). I'll add some.
>
>
>
> On Tue, Nov 25, 2014 at 6:18 PM, Rob Mensching <r...@firegiant.com> wrote:
>
> BVariantSetVariant() isn’t bad. Maybe, BVariantAssign()? I think the
> important trick is to control encryption state, right? Almost want a
> tri-state enum:
>
>
>
> HRESULT BVariantAssign(
>
> __in BURN_VARIANT* pTarget,
>
> __in BURN_VARIANT* pSource,
>
> __in BURN_VARIANT_ENCRYPTION state // unencrypted, encrypted, follow
> source
>
> );
>
>
>
> …or something like that.
>
>
>
>
>
> Note2: Now that you mention it, I like it.
>
>
>
> _______________________________________________________________
>
> FireGiant | Dedicated support for the WiX toolset |
> http://www.firegiant.com/
>
>
>
> *From:* Sean Hall [mailto:r.sean.h...@gmail.com]
> *Sent:* Tuesday, November 25, 2014 1:53 PM
> *To:* WiX toolset developer mailing list
> *Subject:* Re: [WiX-devs] Just opened bug 4609 against v3.9
>
>
>
> My original idea was to create a BVariantSetVariant(__in BURN_VARIANT*
> pVariant, __in BURN_VARIANT* pValue), but I scrapped it since it was almost
> an exact copy of BVariantCopy. Maybe that was the right way to go? I just
> wish I was right on the first pull request way back when :)
>
>
>
> Hmm, I wonder if I introduced that inefficiency in the XML parsing?
> Hopefully not.
>
>
>
> Note2: That's why I didn't submit it in a pull request :) I find it's
> much easier to talk about code changes when you can actually see it.
>
>
>
>
>
> On Tue, Nov 25, 2014 at 3:35 PM, Rob Mensching <r...@firegiant.com> wrote:
>
> Ahh, yes, right. The root issue is that the Source encryption state is
> rarely set “correctly”. I 100% agree that using the target’s encryption
> state is weird. I guess what is needed is a “BVariantCopyEx()” (better
> name’s welcome) that is what you had originally to specify the final
> desired encryption state (i.e. you were right).
>
>
>
> I still stand by my comment though that the variables parsing from XML
> using variant as it does is pretty inefficient. <wink/>
>
>
>
>
>
> Note: the FireGiant change was the direct fix that was tested to unblock
> other processes happening here.
>
>
>
>
>
> Note2: The comment “// Encryption here (and decryption later inside
> BVariantCopy) could have been optimized away with breaking change.” is
> going look very strange over time. It’s much better for comments to explain
> why something was done vs. explaining that something that was not done
> could have been done. The latter will be very confusing (or frustrating)
> trying to figure out what the actual issue is. <smile/>
>
>
>
>
>
> _______________________________________________________________
>
> FireGiant | Dedicated support for the WiX toolset |
> http://www.firegiant.com/
>
>
>
> ------------------------------------------------------------------------------
> Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
> from Actuate! Instantly Supercharge Your Business Reports and Dashboards
> with Interactivity, Sharing, Native Excel Exports, App Integration & more
> Get technology previously reserved for billion-dollar corporations, FREE
>
> http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
> _______________________________________________
> WiX-devs mailing list
> WiX-devs@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/wix-devs
>
>
>
>
> ------------------------------------------------------------------------------
> Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
> from Actuate! Instantly Supercharge Your Business Reports and Dashboards
> with Interactivity, Sharing, Native Excel Exports, App Integration & more
> Get technology previously reserved for billion-dollar corporations, FREE
>
> http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk
> _______________________________________________
> WiX-devs mailing list
> WiX-devs@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/wix-devs
>
>
------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
WiX-devs@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/wix-devs
