Hi, On Tue, Oct 8, 2013 at 4:41 PM, Guillaume "Louis-Marie" Delhumeau < [email protected]> wrote:
> Thanks for all your answers. > > I continue to iterate until we reach something nice. You can see the work > there: > > https://github.com/gdelhumeau/xwiki-platform/tree/feature-wiki-properties-group/xwiki-platform-core/xwiki-platform-wiki/xwiki-platform-wiki-api/src/main/java/org/xwiki/wiki > > I think my mistakes until now was the fact that I didn't want to deviate > too much from the wiki-descriptor module of Vincent. But now, I understand > it is better to make something new. > > Now, the API has a complete javadoc. > > = Modularity = > == Reduce responsibility == > > The first advantage of creating several modules is to reduce the > responsibility of each module. I want one API to handle the wiki creation > and basic management, one API for the template feature (which could be > optional as soon as we have flavors), This transition will most certainly require some considerable refactoring and possibly some method deprecating, for which we already have a strategy, at least for the workspaces UI. Basically, you`d right now be using a $services.wiki.createWiki('newWiki') to create the wiki followed by a $services.wikiTemplate.apply('someWikiTemplateName', 'newWiki') to initialize the wiki with content. When we`d have flavours, you`d probably follow the creation step with a $services.extension.install('someFlavourExtensionID', 'newWiki') to initialize the wiki with the content of a flavour extension. We`d probably also want to keep the option of importing the content from a XAR, so that might be a option too. So this wikiTemplate service would probably contain some methods like: - create/convert (since the argument would probably be a wikiID to mark as template. The content of that wiki would have previously be initialized from an extension of from a XAR) - list/getAll (wikis marked as templates) - delete/revert (remove the template mark, resulting in a regular wiki once again) - isTemplate (wikiID) - apply (as seen above, copy the content to an empty wiki) Side Note1: It could get a bit ugly fast when having to list wikis. What do you do with templates? What do users see, what do admins see if you plan to have a single view(UI) for everything? Probably 2 UIs would fit best (similar to what we have not with WikiManagerUI and WorkspacesUI). The WikiManager part would probably go in the main wiki's administration (for admins) and the WorkspacesUI part would be in the main wiki's WikiIndex (for users). Side Note2: While thinking about templates, I thought that we might need some "copy" and "rename" methods for the WikiManager API, since they might operations that the user might want to perform without having to use templates or some other weird operations. and one API for the ability to > manager users (both the farm and the workspaces use-cases). Do you mean operations like: - join - leave - invite - acceptInvitation - cancelInvitation - getMembers - isMember - etc, all the stuff that it's now written as velocity code in the workspaces-ui pages? If so, than this could be indeed a useful script service to lighten up the velocity pages that are full of duplicate code. However, for the farm use case I don`t see any applications of this service, since the farm use case would be using the same tools that are currently available to the main wiki. I think it's a > good thing to avoid creating a new oldcore. I know this module is too > simple yet to be comparable with oldcore, but I try to design things in > this way. > == Reduce dependencies == > > The second advantage, is that I try to reduce the dependencies for each > module. That means, if a module only needs the list of wikis (and a lot of > modules needs it), they won't have plenty of dependencies. > > For example, the extension manager needs the list of all wikis, but doesn't > care about theirs users, the templates, etc... So it does not need to > depend on the users module, etc... > > == About WikiPropertiesGroup == > > The idea behind this new class is to easily add custom properties in the > descriptor for new modules. Each module provides its own > WikiPropertiesGroupProvider that load and save custom properties for the > wiki. Then, it would be easy to get a property: > wikiDescriptor.get("template").get("isTemplate") or (in velocity) > $wikiDescriptor.template.isTemplate. But we can also create typed wrappers > that access to theses properties. > Care to provide other (practical) examples of where such properties might be useful? > > Actually, in the WikiManager, we have a cache that avoid to look at the > database every time a module needs a descriptor. By using theses properties > groups, we can extend what a descriptor is and make good use of the cache > without re-implementing it in every modules. > > = Misc = > * I have replace getByWikiId() by getById() and so on... > * getAll() will now return the main wiki as well. > * Aliases are now normal string, and the default alias is stored in the > same list than the other alias. > * WikiDescriptor is now the name used, not "Wiki". > * I have added an hidden field in the descriptor. > * I have added an ownerId field in the descriptor. > * I have added a main page field in the descriptor. > * We may need a WikiManager#getMainWiki(), WDYT? > We have $xcontext.mainWikiName that we use everywhere. Maybe it won`t hurt to have a "cleaner" way of getting the main wiki's descriptor/name without relying on the deprecated XWikiContext. Thanks, Eduard P.S.: Excuse my rant above on technical details/API. In the absence of better details, I had to imagine/picture it for myself to better understand what you meant :) * There is no implementation for all of this, right now, I'm waiting for > the API to be defined. > > Thanks, > Louis-Marie > > > > > 2013/10/8 Thomas Mortagne <[email protected]> > > > On Tue, Oct 8, 2013 at 8:53 AM, Marius Dumitru Florea > > <[email protected]> wrote: > > > Hi Guillaume, > > > > > > I also don't see the point of a typed WikiAlias for now and in > > > Wiki#getWikiId() the "Wiki" is redundant. Why not simply Wiki#getId() > > > and Wiki#getAlias()? > > > > > And what is the difference between 'wikiAlias' > > > and 'descriptorAliases' (no javadoc)? If a wiki can have multiple > > > aliases then why not keep just a list and handle the first item > > > differently if needed (like ServletRequest#getParameter() vs. > > > ServletRequest#getParameterValues()). > > > > Same for me, this does not make much sense. The wiki has a list of > > aliases and the URL factory happen to use the first one. Either you > > keep only the list of you rename wikiAlias into default alias to be > > extra safe on what should be used when generating a URL but in all > > cases the list should contain all aliases, the default/first one > > included. > > > > > > > > In WikiManager#getAll() javadoc I see "(except the main one)".Is this > > > the only method that must treat (by contract) the main wiki > > > differently? How do getById() and exists() behave for instance > > > regarding the main wiki? > > > > > The javadoc on an interface / API is very > > > important as it indicates what the implementation should do. So it > > > must state very clear any special use cases. > > > > +1 the most important is the Javadoc, you would probably get less > > questions ;) > > > > > > > > Hope this helps, > > > Marius > > > > > > On Mon, Oct 7, 2013 at 6:17 PM, Guillaume "Louis-Marie" Delhumeau > > > <[email protected]> wrote: > > >> Thanks to your advices (Eddy and Thomas), I propose you a clean > version: > > >> > > > https://github.com/gdelhumeau/xwiki-platform/tree/new-wiki-api/xwiki-platform-core/xwiki-platform-wiki/xwiki-platform-wiki-api/src/main/java/org/xwiki/wiki > > >> > > >> I have only changed the API, not the implementation. > > >> > > >> I think it starts to be clean. > > >> > > >> > > >> 2013/10/7 Guillaume "Louis-Marie" Delhumeau <[email protected]> > > >> > > >>> Hi Eddy. > > >>> > > >>> First, thanks for your answer. > > >>> > > >>> 2013/10/7 Eduard Moraru <[email protected]> > > >>> > > >>>> Hi Guilllaume, > > >>>> > > >>>> 1) What is the point of the WikiDescriptorAlias class if it just > > holds and > > >>>> returns a string? > > >>>> > > >>> > > >>>> Also, the only method in the WikiDescriptorManager that works with > > aliases > > >>>> accepts a String instead of the WikiDescriptorAlias class, so this > > means > > >>>> that the class is useless: > > >>>> > > >>>> WikiDescriptor getByWikiAlias(String wikiAlias) throws > > >>>> WikiManagerException; > > >>>> > > >>> > > >>> I did not really think about it, I just take the work done by Vincent > > on > > >>> wiki descriptors. > > >>> > > >>> > > >>>> 2) WikiManager: > > >>>> > > >>>> The methods wikiIdExists and and isWikiIdAvailable do the same > thing, > > they > > >>>> just call it differently. Use only one, like wikiExists(). > > >>>> > > >>> > > >>> I should have added the javadoc about that. This is what I have in > > mind : > > >>> - wikiExists(String wikiOd) as you said > > >>> - wikiAvailable(String wikiId) that checks if the wiki id is valid > and > > >>> free (that means that there is no existing database with that name -- > > not > > >>> necessary related to XWiki). > > >>> > > >>> It is not implemented yet. > > >>> > > >>> > > >>>> > > >>>> Also, since it's a WikiManager, so the "wiki" part is in the name of > > the > > >>>> class, we get it that it deals with wikis. You could remove the > "wiki" > > >>>> part > > >>>> from method names, like: > > >>>> - create instead of createWiki > > >>>> - delete instead of deleteWiki > > >>>> - exists instead of existsWiki > > >>>> - so on... > > >>>> > > >>> > > >>> I agree. I'll change it ;) > > >>> > > >>> > > >>>> > > >>>> Note on the naming of methods: > > >>>> I`ve noticed a rather annoying thing in the way the methods are > > named. So > > >>>> you have a class that is say... WikiDescriptor. > > >>>> > > >>> > > >>> Indeed, but a Wiki class would not contain something else than a > > >>> descriptor. I don't know what to do. Renaming WikiDescriptor to Wiki? > > >>> > > >>> > > >>>> > > >>>> 3) WikiManager: > > >>>> > > >>>> What is the point of these 2 methods? > > >>>> > > >>>> void setDescriptor(WikiDescriptor descriptor); > > >>>> > > >>>> void removeDescriptor(WikiDescriptor descriptor); > > >>>> > > >>>> Looking at the implementation, I see that you`re handling > descriptors > > >>>> (setting/deleting) whithout corelating it to the wikis. I mean, if > you > > >>>> remove a descriptor, then you must remove the wiki too, and that is > > the > > >>>> task of the WikiManager.delete() method. > > >>>> > > >>> > > >>> I have keeped them because it is used in the descriptors > > implementation of > > >>> Vincent. But I agree that it is weird to have it in the API. > > >>> > > >>> > > >>>> > > >>>> 4) Don`t use implementation details. Improve the interface (API) if > > there > > >>>> is information you`re missing: > > >>>> > > >>>> > > > https://github.com/gdelhumeau/xwiki-platform/blob/new-wiki-api/xwiki-platform-core/xwiki-platform-wiki/xwiki-platform-wiki-default/src/main/java/org/xwiki/wiki/descriptor/internal/DefaultWikiManager.java#L209 > > >>>> > > >>> > > >>> I disagree. > > >>> In this code, I need to have the document related to the descriptor. > It > > >>> depends on the current implementation, because we can imagine an > > >>> implementation where the descriptors are not stored in the wiki. So I > > won't > > >>> add a getDocument() in the API, but I need it in my implementation. > > >>> > > >>> > > >>>> > > >>>> 5) DefaultWikiManager.createDescritptor > > >>>> Since you have a WikiDescriptorBuilder class, maybe that is where > the > > >>>> logic > > >>>> of creating/building the descriptor's details (objects, document) > and > > >>>> where > > >>>> the logic of extracting the document name from wiki name and > viceversa > > >>>> should be located. > > >>>> > > >>>> > > > https://github.com/gdelhumeau/xwiki-platform/blob/new-wiki-api/xwiki-platform-core/xwiki-platform-wiki/xwiki-platform-wiki-default/src/main/java/org/xwiki/wiki/descriptor/internal/DefaultWikiManager.java#L126 > > >>>> > > >>> > > >>> I agree, I will move that code into WikiDescriptorBuilder. > > >>> > > >>> > > >>>> > > >>>> 6) DefaultWikiManager.getByWikiId > > >>>> Again seeing the logic of wikiID-to-document. It should be located > in > > a > > >>>> single place and not spread in multiple places. Use a method of the > > >>>> WikiManager class or of the DescriptorBuilder class. > > >>>> > > >>> > > >>> OK. > > >>> > > >>> > > >>>> > > >>>> 7) WikiManagerScriptService: > > >>>> - deleteWiki: Why do you need Admin right to delete a wiki when you > > did > > >>>> not > > >>>> need it in order to create it? That`s bad. We should probably have a > > >>>> deleteWiki right as well. > > >>> > > >>> > > >>> It is a temporary solution in my implementation. > > >>> The problem is: who has the right to remove a wiki ? > > >>> - his owner? But we don't have the notion of owner in this API. It > > belongs > > >>> to the wiki-users module to handle this concept. > > >>> - someone who has the CREATE_WIKI right? That means every person who > > has > > >>> the CREATE_WIKI right can also delete any existing wiki? > > >>> - someone who as the DELETE_WIKI right? Why not, but we need to make > a > > >>> vote for adding this new right. > > >>> > > >>> So, in my first implementation, I have used the admin right. > > >>> > > >>> > > >>>> Also, consider the workspaces use case when user > > >>>> create and delete wikis/workspaces when they want to and when they > are > > >>>> done > > >>>> with them. > > >>>> > > >>> > > >>> To me, it belongs to the future wiki-user module. > > >>> > > >>> > > >>>> - context key for exceptions "lastexception2" is not the best of > > choices. > > >>>> Try something like "wikiException" instead, or even just > > "lastexception" > > >>>> like it was before (if we consider this as a best practice). What's > > the > > >>>> reason for "lastexception2"? > > >>>> > > >>>> > > >>> It's a bad commit. I have setted this name for debugging reason. > > >>> > > >>> > > >>>> > > >>>> As a general note, I fail to see the added value of this module at > > this > > >>>> point. I mean, the whole point was to have both the functionality of > > >>>> WikiManager and Workspaces. I get it that it's a work in progress > and > > that > > >>>> it's at the first stages, but you need to touch those points before > it > > >>>> becomes relevant. Not that it's bad, just that it's not much there > to > > see > > >>>> at this point. > > >>>> > > >>> > > >>> The idea is to split in different simple modules, that enable us to > > easily > > >>> add a new feature or to depends on a module without depending on the > > whole > > >>> core. I try to reduce the dependencies while I design the modules. > > >>> > > >>> > > >>>> > > >>>> Hope this helps, > > >>>> Eduard > > >>>> > > >>>> > > >>> Thanks Eduard! > > >>> > > >>> Louis-Marie > > >>> > > >>> > > >>>> > > >>>> On Mon, Oct 7, 2013 at 11:49 AM, Guillaume "Louis-Marie" Delhumeau < > > >>>> [email protected]> wrote: > > >>>> > > >>>> > Hi. > > >>>> > > > >>>> > In this thread, I want to propose you the tiniest API that we need > > to > > >>>> > handle multiwiki. All other features (users, templates, > > workspaces...) > > >>>> will > > >>>> > be on other modules, because I think it is better for the > > extensibility. > > >>>> > > > >>>> > The new module will be based on the > > xwiki-platform-wiki-descriptor-api > > >>>> > module, that I will rename to xwiki-platform-wiki-api. The > > >>>> > WikiDescriptorManager becomes WikiManager and handle both > > descriptors > > >>>> and > > >>>> > databases. > > >>>> > > > >>>> > This API will permit: > > >>>> > * to create a wiki > > >>>> > * to remove a wiki > > >>>> > * to list all wikis (returning a list of descriptors) > > >>>> > * ... > > >>>> > > > >>>> > You can see the code of that proposal there: > > >>>> > > > >>>> > > > >>>> > > > https://github.com/gdelhumeau/xwiki-platform/tree/new-wiki-api/xwiki-platform-core/xwiki-platform-wiki > > >>>> > > > >>>> > The most important is to decide what the API must look like. The > > >>>> > implementation can still be modified afterwards. > > >>>> > > > >>>> > I hope you like it, > > >>>> > > > >>>> > Louis-Marie > > >>>> > _______________________________________________ > > >>>> > devs mailing list > > >>>> > [email protected] > > >>>> > http://lists.xwiki.org/mailman/listinfo/devs > > >>>> > > > >>>> _______________________________________________ > > >>>> devs mailing list > > >>>> [email protected] > > >>>> http://lists.xwiki.org/mailman/listinfo/devs > > >>>> > > >>> > > >>> > > >> _______________________________________________ > > >> devs mailing list > > >> [email protected] > > >> http://lists.xwiki.org/mailman/listinfo/devs > > > _______________________________________________ > > > devs mailing list > > > [email protected] > > > http://lists.xwiki.org/mailman/listinfo/devs > > > > > > > > -- > > Thomas Mortagne > > _______________________________________________ > > devs mailing list > > [email protected] > > http://lists.xwiki.org/mailman/listinfo/devs > > > _______________________________________________ > devs mailing list > [email protected] > http://lists.xwiki.org/mailman/listinfo/devs > _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
