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.

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

Add dialog ext functions and DialogBox wrapper #41

Merged
merged 8 commits into from Jan 19, 2018

Conversation

Projects
None yet
4 participants
@Jampi0n
Copy link
Contributor

Jampi0n commented Jan 17, 2018

Allows dot-syntax for dialogs.
Package is comparable to other packages of the _handles folder.

add Dialog.wurst
Allows dot-syntax for dialogs.
Package is comparable to other packages of the _handles folder.
@IgorSamurovic

This comment has been minimized.

Copy link
Contributor

IgorSamurovic commented Jan 17, 2018

Looks solid. It seems to wrap all functionalities of dialogs (if I am not mistaken). You can possibly extend some functionalities to make it even easier to use or solve some typical dialog issues, but obviously, you don't need to add something that nobody (including yourself) would use. So don't be afraid to add more functionalities if you believe they could make this wrapper even more useful.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Jan 17, 2018

Cool 😎
It would be neat to have some hot doc /** example */ above at least some of the functions, like addQuitButton to elaborate on it's function (does quit refer to quitting the dialog, or quitting the game?) as well as a small package description with a few known pitfalls (dialogs pause the game in SP, but not MP iirc and the user is unable to chat because enter acts as pressing a button on the dialog)

@Jampi0n

This comment has been minimized.

Copy link
Contributor Author

Jampi0n commented Jan 17, 2018

Thank you. Yes this is just for better syntax of dialog natives.
I made a more advanced dialog system using two classes DialogBox and DialogButton to make dialogs easier to use.
The idea is to only create the DialogBox with a title and adding buttons to it with text and a function to run, if that button is clicked.

        DialogBox db = new DialogBox("Example dialog")
	db.addButton("Button 1", ()-> print("Button 1 clicked") )
	db.display(players[0], true)

This is probably too advanced for a basic package from the standard library though.

I am no expert of dialogs at all. I simply made this package for the better syntax. I decided to do all dialog natives while I am at it, so I can't say what addQuitButton does right now.
I will investigate on it and add descriptions to the package.

edit: not sure how to post code with line breaks here, now fixed

@IgorSamurovic

This comment has been minimized.

Copy link
Contributor

IgorSamurovic commented Jan 17, 2018

This is probably too advanced for a basic package from the standard library though.

Nope, not at all. The standard library isn't about "basic packages", it is about encapsulating functionality in the best way possible. If something's generally useful, it should be part of it. Adding lambda functionality like this is actually very helpful, especially if you write proper hotdoc alongside it.

So if you ask me, you can add this lambda based functionality, since the users would probably use something like that anyways. I know I would, if I ever worked with dialogs.

@Jampi0n

This comment has been minimized.

Copy link
Contributor Author

Jampi0n commented Jan 17, 2018

Ok, I will try to clean up that code a bit. Getting feedback on it will be very useful for me, as I just started using wurst.
This function works with both lambdas and normal functions, but I agree here lambas are very useful, because most of these functions are used only in this context anyway.

@IgorSamurovic

This comment has been minimized.

Copy link
Contributor

IgorSamurovic commented Jan 17, 2018

I am actually not too sure that you need a wrapper class for this, I think working with the basic dialog should do just fine, especially if you just use a HashMap<button, closure> to work with it.

The rule of the thumb is not to create a wrapper class until you really stand to gain something from it, which in this case, you really don't.

Unless I am forgetting how dialogs work and you need a trigger for every dialog in which case this might get ugly.

Well, post it, explain your reasoning for decisions and we'll see. In any case "new Dialog" does look better than createDialog() called in the global scope, obviously, so I think this could be fine at any rate. But that also means you would map this functionality to the wrapped Dialog object, and not dialog.

DialogDisplay(whichPlayer, this, flag)

public function dialog.setMessage(string messageText)
DialogSetMessage(this, messageText)

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jan 17, 2018

Contributor

missing eof lf

@Cokemonkey11

This comment has been minimized.

Copy link
Contributor

Cokemonkey11 commented Jan 17, 2018

I agree that the code is good as-is. Extension functions for natives is almost always useful.

I also think a wrapper API would be nice, though not sure about the design, and definitely not needed to have instead of this. A wrapper API should use the extension-functions API.

DialogBox
DialogBox lets you to create dialogs and associate the buttons with code.
@Jampi0n

This comment has been minimized.

Copy link
Contributor Author

Jampi0n commented Jan 17, 2018

I added the DialogBox class.
I am not happy with the number of data structures I needed to make this rather simple system work.
Especially regarding the code parameter I think it can be improved, so I don't need to create one trigger per button.

DialogSetMessage(this, messageText)


public class DialogBox

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

belongs in a separate file


public class DialogBox

private static HashMap<button,trigger> buttonTriggers = new HashMap<button,trigger>

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

<button,trigger> -> <button, trigger>
for all cases, as well as
private static HashMap<button,trigger> buttonTriggers ->private static constant buttonTriggers

private static HashMap<dialog,DialogBox> dialogBoxes = new HashMap<dialog,DialogBox>
private dialog dialogHandle
private trigger clickDialog
private string title

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

if this is private, why does it exist? you don't even use it.

construct(string title)
this.title = title
this.dialogHandle = createDialog()
this.dialogHandle.setMessage(title)

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

use dot-dot ".." syntax

this.clickDialog = CreateTrigger()
this.clickDialog.registerDialogEvent(this.dialogHandle)
this.clickDialog.addAction(function removeClickedDialog)
this.buttons = new LinkedList<button>

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

inline these assignments into member definition

static function removeClickedDialog()
button b = GetClickedButton()
buttonTriggers.get(b).execute()
destroy(dialogBoxes.get(GetClickedDialog()))

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

remove brackets, this makes it look like you#re calling a function

function addButton(string buttonText, integer hotkey, code cb)
button b = this.dialogHandle.addButton(buttonText, hotkey)
this.buttons.add(b)
trigger t = CreateTrigger()

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

again, use let and .. and proper names

this.dialogHandle.display(whichPlayer, flag)

ondestroy
while(not buttons.isEmpty())

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

use a for each loop, either for from with iterator and using remove, or simple .forEach().
Dialogs aren't performance critical so we aim for most readable, concise and simple code.

This comment has been minimized.

@IgorSamurovic

IgorSamurovic Jan 17, 2018

Contributor

Or you can just do

for btn in buttons
    -- do stuff ---
destroy buttons

since you need to destroy this anyways, no need to individually clear things.

button b = buttons.pop()
trigger t = buttonTriggers.get(b)
buttonTriggers.remove(b)
t.clearActions()

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

.. use cascade

buttonTriggers.remove(b)
t.clearActions()
t.destr()
destroy(buttons)

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

omit brackets

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Jan 17, 2018

I have mostly marked convention and readability issues for now and not design. In any case it is missing hot doc documentation on the usage and for the functions.

You should make more use of type inference, cascade op, not add unnecessary brackets and rely on higher level abstraction in non performance critical situations.
Also please no 1 letter variable names.



public class DialogBox

This comment has been minimized.

@IgorSamurovic

IgorSamurovic Jan 17, 2018

Contributor

could be just Dialog I think. Or it can be DialogBox, in a new wurst file called DialogBox.wurst.

function addButton(string buttonText, code cb)
this.addButton(buttonText, 0 , cb)

This comment has been minimized.

@IgorSamurovic

IgorSamurovic Jan 17, 2018

Contributor

Add the action that calls removeClickedDialog on addButton functions. This way the user does not have to use it explicitly. Also make removeClickedDialog a private static function.

this.dialogHandle.display(whichPlayer, flag)

ondestroy
while(not buttons.isEmpty())

This comment has been minimized.

@IgorSamurovic

IgorSamurovic Jan 17, 2018

Contributor

Or you can just do

for btn in buttons
    -- do stuff ---
destroy buttons

since you need to destroy this anyways, no need to individually clear things.

Update DialogBox
Seperate file for DialogBox and overall improvements.
@Jampi0n

This comment has been minimized.

Copy link
Contributor Author

Jampi0n commented Jan 17, 2018

Thank you for all the feedback. I tried to adress all the issues as far as I understood them.
I added a little bit hot doc documentation.
I will improve it and reply to some of your comments once I have more time.

@@ -0,0 +1,67 @@
package DialogBox

This comment has been minimized.

@IgorSamurovic

IgorSamurovic Jan 17, 2018

Contributor

Should probably be in wurst/util/ instead of _handles, since it's kinda reserved for primitive, wc3 types.

@IgorSamurovic
Copy link
Contributor

IgorSamurovic left a comment

Overall, better. Got a few tidbits to fix, but frotty will comment on that with an explanation.

@Frotty
Copy link
Member

Frotty left a comment

Good, now just augment with closures and it should be done 👍
Also please add a space after /** as is in other packages.
Thanks for the work!

/**DialogBox lets you to create dialogs and associate the buttons with code.*/
public class DialogBox

private static constant HashMap<button, trigger> buttonTriggers = new HashMap<button, trigger>

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

Hey you got this wrong 😆 . Not "add" constant everywhere, but replace the type definition with constant.
so
private static constant HashMap<button, trigger> buttonTriggers = new HashMap<button, trigger>
to
private static constant buttonTriggers = new HashMap<button, trigger>
this gets rid of needing generic params twice and works just like let/var

This comment has been minimized.

@Jampi0n

Jampi0n Jan 18, 2018

Author Contributor

Yes that is nice. Writing all the generic parameters started to get annyoing.

Still most of the member variables of that class are constant.

private constant dialog dialogHandle = createDialog()
private constant trigger clickDialog = CreateTrigger()
..registerDialogEvent(this.dialogHandle)
..addAction(function removeClickedDialog)

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

I only ment the Create part.
Move

..registerDialogEvent(this.dialogHandle)
..addAction(function removeClickedDialog)

back into constructor.
Change ..addAction(function removeClickedDialog) to ..addAction(() -> removeClickedDialog())

This comment has been minimized.

@Jampi0n

Jampi0n Jan 18, 2018

Author Contributor

Oh ok, I was already wondering, if executing functions there makes sense.

What is the benefit of using () -> compared to function in this context?

public function dialog.addButton(string buttonText, integer hotkey) returns button
return DialogAddButton(this, buttonText, hotkey)

/**If clicked, quits the game*/

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

Should be more like "adds a quit button to this dialog. If it is clicked, it ends the game for that player(?)"

public function dialog.addQuitButton(boolean doScoreScreen, string buttonText, integer hotkey) returns button
return DialogAddQuitButton(this, doScoreScreen, buttonText, hotkey)

/**removes all buttons from a dialog*/

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

capitalize r

this.addButton(buttonText, 0 , cb)

/**Hotkey: use ASCII number of the capital letter.*/
function addButton(string buttonText, integer hotkey, code cb)

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

The general consensus here should be to use a closure instead of code, so the user does not have to worry about carrying over data.
Imagine following freehand example (didnt test):
https://bin.wurstlang.org/opicaqoguq.wurst

With the use of closures, the class instance and therefore buyingUnit are automatically available inside the lambda expression, which is not the case with code.
In that case the user would have to attach his data manually somehow.
Using closures would also get rid of triggers completely.


destroy buttons
dialogBoxes.remove(this.dialogHandle)
this.dialogHandle.clear()

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

use cascade .. :)

This comment has been minimized.

@Jampi0n

Jampi0n Jan 18, 2018

Author Contributor

The cascade .. is something new for me. It takes time to get used to it, but it's certainly useful.


private static function removeClickedDialog()
let btn = GetClickedButton()
buttonTriggers.get(btn).execute()

This comment has been minimized.

@Frotty

Frotty Jan 17, 2018

Member

buttonTriggers.get(btn) could be null. Should check if the button has no attached listener and throw an error if so.

This comment has been minimized.

@Jampi0n

Jampi0n Jan 18, 2018

Author Contributor

It can be intended to create buttons without a listener. If you simply want a button to cancel the dialog, no code needs to be executed.
The same goes for the quit button, though I am not sure it would even be used.

@Frotty Frotty changed the title add Dialog.wurst Add dialog ext functions and DialogBox wrapper Jan 17, 2018

@Frotty Frotty self-assigned this Jan 17, 2018

updated DialogBox moved to util
Updated DialogBox according to comments from the Pull Request.
DialogBox is now part of util, as _handles is preserved for wc3 types.
Quit Button added to the class.
Uses Closures instead of triggers.
@Jampi0n

This comment has been minimized.

Copy link
Contributor Author

Jampi0n commented Jan 18, 2018

There is still one conceptual problem I don't know how to deal with at the moment.
The DialogBox has the functionality to automatically destroy itself when clicked. This is a good feature in singleplayer mode, but does not work for multiplayer dialogs.

To solve this I could use the display function to keep track of the players who can see/click the dialog. Once everyone of them has clicked the dialog, it will be destroyed.
Of course the map maker can use destroy to manually destroy the object at any time.

In general I need to do multiplayer tests of the system to be able to tell if it performs as intended in multiplayer as well.

@IgorSamurovic
Copy link
Contributor

IgorSamurovic left a comment

Overall, I am happy to say that I have mostly nitpicks, and this is already more than serviceable.

import Dialog
import LinkedList

public interface Callback

This comment has been minimized.

@IgorSamurovic

IgorSamurovic Jan 18, 2018

Contributor

The name is too generic, especially so since it's a public interface. Since it looks like it is only used internally, you could argue that it can simply not be public, however, considering that it is used as an argument (which can also be created and saved outside this system as variables), it should remain public, but change name, to something like DialogBoxButtonClosure. It looks ugly and overly verbose, but closure names are typically long and descriptive because they both kind of need to be in public space, and need to be and sound specific enough in order not to clash with others. Luckily, users rarely need to manually write them down, so they're mostly bound to systems they are part of.

buttons.forEach( (button btn) -> begin
let cb = buttonClosures.get(btn)
buttonClosures.remove(btn)
if(cb!=null)

This comment has been minimized.

@IgorSamurovic

IgorSamurovic Jan 18, 2018

Contributor

if cb != null

buttonClosures.put(btn, cb)

/** Toggles visibility of the DialogBox for a player. Dialogs are invisible by default
Dialogs cannot be shown at map initialization */

This comment has been minimized.

@IgorSamurovic

IgorSamurovic Jan 18, 2018

Contributor

Add dot at the end of sentences, if that's how the whole package is documented. Sadly, we haven't yet made proper standards for how this is done (whether there's a dot or not), but having it consistent on a package level will make it easier to refactor later when standards are decided.

private static function removeClickedDialog()
let btn = GetClickedButton()
let cb = buttonClosures.get(btn)
if(cb!=null)

This comment has been minimized.

@IgorSamurovic

IgorSamurovic Jan 18, 2018

Contributor

No need for braces, if cb != null will suffice.

function addButton(string buttonText, integer hotkey, Callback cb)
let btn = this.dialogHandle.addButton(buttonText, hotkey)
this.buttons.add(btn)
buttonClosures.put(btn, cb)

This comment has been minimized.

@IgorSamurovic

IgorSamurovic Jan 18, 2018

Contributor

You can't add a null to a LinkedList<T>, so this will throw an error. Check if cb is null. Unless, of course, this is intended behavior and adding null is an actual error (but I got a different idea from your argument that a user can willfully pass null). In that case, create additional addButton functions that take only text, and text and hotkey. Don't be afraid to overload functions many times if it makes the user not have to pass null to accomplish something. Even if the code looks a bit messier in the system, it will look neater in the user code, which is what you should aim for if you have 0-4 parameters like here.

This comment has been minimized.

@Jampi0n

Jampi0n Jan 18, 2018

Author Contributor

Thank you for your insight. I was considering overloading the function, but I was not sure if using null as parameter would be easier.
The problem here is, that there are 3 optional paramters, so this will be a lot of functions if every combination is a new function.
The linked list only contains the buttons, which are never null. Only the hashmap maps buttons to closures and the closure could be null. Can you put null values into hashmaps? At least there was not error message when I just tested it using a null closure.

This comment has been minimized.

@IgorSamurovic

IgorSamurovic Jan 18, 2018

Contributor

That's fine I guess, I didn't realize at first that it's a map.

Still, there are really not 3 optional parameters - there are only two. You will always need to name the button, for the dialog to make sense to the player. This means that the optional parameters are going to be the hotkey and callback. This makes for a 2 * 2 = 4 functions which is okay to manage. When overloading a function it's important to think of use cases, and not putting a text on a button is really not one of those. If the user really wants to, he can enter an empty string, but it is not going to be desired in probably any case. So that option is still open, it is just less convenient and obviously discouraged.

This comment has been minimized.

@Jampi0n

Jampi0n Jan 18, 2018

Author Contributor

Yes, I agree. I wasn't thinking about removing the string paramter, but somehow thought there were 3 other paramters. Probably because the quit button contains one additional paramter.

@IgorSamurovic

This comment has been minimized.

Copy link
Contributor

IgorSamurovic commented Jan 18, 2018

I guess multiplayer is kind of posing a problem.That'll require additional testing to figure out how to solve, though, since we are generally inexperienced with dialogs, and especially so in multiplayer.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Jan 19, 2018

As we discussed, just let the destroying/closing be done by the user.
Often dialogs are reused anyways.
If you make these changes, I will merge @Jampi0n

Jampi0n added some commits Jan 19, 2018

Removed Auto Destroy
DialogBox no longer destroys itself when clicked.
Improved Documentation
Minor changes:
changed closure name
more readable syntax
Improved Documentation:
Package, Class and functions.
@Jampi0n

This comment has been minimized.

Copy link
Contributor Author

Jampi0n commented Jan 19, 2018

Changes have been made.
I also improved the documentation and added an example use for the class.
As suggested I overloaded the addButton functions for the optional parameters hotkey and closure.

@Frotty

Frotty approved these changes Jan 19, 2018

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Jan 19, 2018

nice 🍉

@Frotty Frotty merged commit e2abd8a into wurstscript:master Jan 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Jampi0n Jampi0n deleted the Jampi0n:dialog branch Jan 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment