Hi Bilgin, For item #1 and #2 - makes sense. I didnt notice the fact that configwrapper instance was intended to be global.
Can #3 and #5 be fixed? Bilgin Ibryam wrote: > > Quoting Ritesh Trivedi <[EMAIL PROTECTED]>: > > Hi Ritesh, comments inline > >> >> Hi, >> >> Have been trying to use configure feature and am encountering several >> limitations (possible bug(s)/issues) with the current implementation. >> >> Both items #1 and #2 causes a bug where if you modify >> ProductConfigWrapper >> the cache instances still remains unchanged as both the methods returns >> deep >> copy of the wrapper. > > In the cache is stored ProductConfigWrapper instances which are not > configured/modified. If you check the cache key, you will see that > there is no indicator for selected options, so it is not possible to > make difference between configured options in the cache. > > >> 1. ProductConfigWrapper.getProductConfigWrapper() not sure why we need >> to call copy constructor again when the configWrapper was just created >> brand new just in the previous statement? >> >> productConfigCache.put(cacheKey, new >> ProductConfigWrapper(configWrapper)); >> >> can just be >> >> productConfigCache.put(cacheKey, configWrapper); >> >> 2. Again ProductConfigWrapper.getProductConfigWrapper() not sure why >> we need to call copy constructor again when an instance from cache >> already exists >> configWrapper = new >> ProductConfigWrapper((ProductConfigWrapper)productConfigCache.get(cacheKey)); >> >> can just be >> >> configWrapper = (ProductConfigWrapper)productConfigCache.get(cacheKey); > > > Changes #1 and #2 will cause to addition to the cache an instance > which will be accessible and configured from the user who added it. > But then every next user using the same cache key (the same store, > catalog and currency) will get the already configured object with > selected options from the first user. > >> >> 3. ConfigOption inner class, needs to expose either configItemId or >> parent configItem - if lets say I store the selected configoptions in >> the session and later time what to mark those items selected in >> ProductConfigWrapper - both the setSelected() methods in >> ProductConfigWrapper needs configItem information and not being able >> to access it from the stored config options forces storage of >> configItem objects too > > I think it is ok to expose parent configItem. > >> >> 4. Also on configOption isMandatory() should be exposed as isSelected >> is exposed on configItem >> > > If configItem is exposed (from point 3) isMandatory() can be called. > As options can't be mandatory, adding isMandatory() to configOption > could be confusing. > > Bilgin > >> -- >> View this message in context: >> http://www.nabble.com/ProductConfigWorker-and-ProductConfigWrapper-multiple-issues---Please-comment-tp19114133p19114133.html >> Sent from the OFBiz - Dev mailing list archive at Nabble.com. >> >> > > > > ---------------------------------------------------------------- > This message was sent using IMP, the Internet Messaging Program. > > > -- View this message in context: http://www.nabble.com/ProductConfigWorker-and-ProductConfigWrapper-multiple-issues---Please-comment-tp19114133p19115335.html Sent from the OFBiz - Dev mailing list archive at Nabble.com.
