Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XRENDERING-764: Add an option for messages to be set as status #319

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

XRENDERING-764: Add an option for messages to be set as status #319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ba18ea
XRENDERING-764: Add an option for messages to be set as status
Sereza7 Dec 2, 2024
9d830a3
XRENDERING-764: Add an option for messages to be set as status
Sereza7 Dec 2, 2024
0ff0d5c
XRENDERING-764: Add an option for messages to be set as status
Sereza7 Dec 2, 2024
fe7c2c7
XRENDERING-764: Add an option for messages to be set as status
Sereza7 Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions ...essage/src/main/java/org/xwiki/rendering/internal/macro/message/AbstractMessageMacro.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
import org.xwiki.rendering.macro.MacroExecutionException;
import org.xwiki.rendering.macro.MacroPreparationException;
import org.xwiki.rendering.macro.box.AbstractBoxMacro;
import org.xwiki.rendering.macro.box.BoxMacroParameters;
import org.xwiki.rendering.macro.descriptor.DefaultContentDescriptor;
import org.xwiki.rendering.parser.ParseException;
import org.xwiki.rendering.parser.Parser;
Expand All @@ -50,7 +49,7 @@
* @version $Id$
* @since 2.0M3
*/
public abstract class AbstractMessageMacro extends AbstractBoxMacro<BoxMacroParameters>
public abstract class AbstractMessageMacro extends AbstractBoxMacro<MessageMacroParameters>
{
private String iconName;

Expand All @@ -77,11 +76,10 @@ public AbstractMessageMacro(String macroName, String macroDescription)
{
super(macroName, macroDescription,
new DefaultContentDescriptor("Content of the message", true, Block.LIST_BLOCK_TYPE),
BoxMacroParameters.class);
MessageMacroParameters.class);
}

@Override
protected List<Block> parseContent(BoxMacroParameters parameters, String content,

protected List<Block> parseContent(MessageMacroParameters parameters, String content,
MacroTransformationContext context) throws MacroExecutionException
{
List<Block> macroContent = getMacroContentParser().parse(content, context, false, context.isInline())
Expand Down Expand Up @@ -118,12 +116,15 @@ public void prepare(MacroBlock macroBlock) throws MacroPreparationException
}

@Override
public List<Block> execute(BoxMacroParameters parameters, String content, MacroTransformationContext context)
public List<Block> execute(MessageMacroParameters parameters, String content, MacroTransformationContext context)
throws MacroExecutionException
{
List<Block> boxFoundation = super.execute(parameters, content, context);
if (!boxFoundation.isEmpty() && getIconName() != null) {
Block defaultBox = boxFoundation.get(0);
if (parameters.isStatus()) {
defaultBox.setParameter("role", "status");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this parameter only used when the icon name is set?

}
// For an easier styling, we wrap the content and title together if they are non-empty and visible
if (defaultBox.getChildren().size() > 1) {
Block boxTextContent = new GroupBlock(defaultBox.getChildren());
Expand Down
62 changes: 62 additions & 0 deletions ...sage/src/main/java/org/xwiki/rendering/internal/macro/message/MessageMacroParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* See the NOTICE file distributed with this work for additional
* information regarding copyright ownership.
*
* This is free software; you can redistribute it and/or modify it
* under the terms of the GNU Lesser General Public License as
* published by the Free Software Foundation; either version 2.1 of
* the License, or (at your option) any later version.
*
* This software is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this software; if not, write to the Free
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
*/
package org.xwiki.rendering.internal.macro.message;

import org.xwiki.properties.annotation.PropertyAdvanced;
import org.xwiki.properties.annotation.PropertyDescription;
import org.xwiki.properties.annotation.PropertyDisplayType;
import org.xwiki.rendering.macro.box.BoxMacroParameters;
import org.xwiki.stability.Unstable;

/**
* Parameters for the Message macro.
*
* @version $Id$
* @since 17.0.0RC1
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the whole class should be marked as @Unstable. Further, macro parameter classes normally aren't internal in XWiki.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, macro parameters being APIs, it's safer to put them under revapi scrutiny.

public class MessageMacroParameters extends BoxMacroParameters
{
/**
* @see #isStatus()
*/
private boolean isStatus;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the getter is isStatus, the field name is generally expected to be status.


/**
* @since 17.0.0RC1
* @return whether or not the current message is a status.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need some more documentation on what exactly a "status" is and why would one set that to true.

*/
@Unstable
public boolean isStatus()
{
return this.isStatus;
}

/**
* @since 17.0.0RC1
* @param isStatus refers to {@link #isStatus()}
*/
@PropertyDescription("Whether or not this message should be announced as a status.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a much better description as otherwise users will be completely lost when this flag should be set. The description should clearly explain what consequences this has and when it should be used. Also, I'm wondering if it could make sense to more generically expose a "role" parameter - or is it clear that there is no other role that could be interesting?

@PropertyAdvanced
@Unstable
public void setStatus(boolean isStatus)
{
this.isStatus = isStatus;
}
}
6 changes: 3 additions & 3 deletions ...ering-macro-message/src/test/java/org/xwiki/rendering/macro/message/MessageMacroTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@
import org.junit.jupiter.api.Test;
import org.xwiki.rendering.block.Block;
import org.xwiki.rendering.block.RawBlock;
import org.xwiki.rendering.internal.macro.message.MessageMacroParameters;
import org.xwiki.rendering.macro.Macro;
import org.xwiki.rendering.macro.MacroExecutionException;
import org.xwiki.rendering.macro.box.BoxMacroParameters;
import org.xwiki.rendering.parser.ParseException;
import org.xwiki.rendering.parser.Parser;
import org.xwiki.rendering.renderer.BlockRenderer;
Expand Down Expand Up @@ -88,7 +88,7 @@ void executeWithRawBlockForIconBlock() throws Exception
IconProvider iconProvider = this.componentManager.registerMockComponent(IconProvider.class);
when(iconProvider.get("information")).thenReturn(new RawBlock("some html", Syntax.HTML_4_01));
Macro messageMacro = this.componentManager.getInstance(Macro.class, "info");
BoxMacroParameters parameters = new BoxMacroParameters();
MessageMacroParameters parameters = new MessageMacroParameters();
when(this.context.getSyntax()).thenReturn(Syntax.XWIKI_2_1);

List<Block> blocks = messageMacro.execute(parameters, "content", this.context);
Expand All @@ -112,7 +112,7 @@ void executeWhenParsingErrorForIconPrettyName() throws Exception
Parser plainTextParser = this.componentManager.registerMockComponent(Parser.class, "plain/1.0");
doThrow(new ParseException("error")).when(plainTextParser).parse(any(Reader.class));
Macro messageMacro = this.componentManager.getInstance(Macro.class, "info");
BoxMacroParameters parameters = new BoxMacroParameters();
MessageMacroParameters parameters = new MessageMacroParameters();
when(this.context.getSyntax()).thenReturn(Syntax.XWIKI_2_1);

Throwable exception = assertThrows(MacroExecutionException.class,
Expand Down