Skip to content

Various improvements #42

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

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

Various improvements #42

wants to merge 20 commits into from

Conversation

iacore
Copy link

@iacore iacore commented Apr 5, 2022

See commit messages for details.

Deprecated non-standard APIs and added standard ones.

PMunch and others added 12 commits February 26, 2017 03:10
Added onselected methods and fields for RadioButtons
… to keep the style between the tabs the same, this allows you to do that more easily
Fix cross-compilation to Windows on Linux using x86_64-w64-mingw32-gcc.
Linux is case- and slash-sensitive, but Windows should be able to handle
both afaik.
* add Grid widget
Fix delete for Box
ui.nim Outdated
proc quit*() = rawui.quit()

proc mainLoop*() =
proc mainLoop*() {.deprecated: "Use 'main' instead".} =
Copy link
Member

Choose a reason for hiding this comment

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

mainLoop is a much better name for this so don't deprecate it.

Copy link
Author

Choose a reason for hiding this comment

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

Done

rawui.uninit()
if rawui.mainStep(0) == 0: break

proc queueMain*(f: proc (): void {.cdecl.}) =
Copy link
Member

Choose a reason for hiding this comment

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

  1. It should be f: proc () {.cdecl.}, the void is implied.
  2. If you're not sure if the "cast works or not" why add the feature? How are you provide the feature if the cast doesn't work?
  3. ui.nim tries to provide an idiomatic API. Otherwise why would we have the rawui and ui separation?

rawui.queueMain(cast[proc (_:pointer): void {.cdecl.}](f), nil) # Not sure if this cast works or not

proc queueMain*[T](f: proc (_:T): void {.cdecl.}, p: T) =
rawui.queueMain(cast[proc (_:pointer): void {.cdecl.}](f), cast[pointer](p))
Copy link
Member

Choose a reason for hiding this comment

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

Don't add procs that you don't know will work, are low level and are against the spirit of the module.

Copy link
Author

Choose a reason for hiding this comment

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

I don't know how to properly wrap a nim proc into a c proc.

@@ -473,11 +502,11 @@ template addMenuItemImpl(ex) =
menuItemOnClicked(result.impl, wrapmeOnclicked, cast[pointer](result))
m.children.add result

proc addItem*(m: Menu; name: string, onclicked: proc()): MenuItem {.discardable.} =
proc addItem*(m: Menu; name: string, onclicked: proc() = nil): MenuItem {.discardable.} =
Copy link
Member

@Araq Araq Apr 11, 2022

Choose a reason for hiding this comment

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

Why is = nil now the default? Which kind of items are not clickable? Why not have an addItemFiller proc instead or something like that?

proc addQuitItem*(m: Menu): MenuItem {.discardable.} =
addMenuItemImpl(menuAppendQuitItem(m.impl))

proc addQuitItem*(m: Menu, shouldQuit: proc(): bool): MenuItem {.discardable, deprecated:"Register menu action yourself".} =
Copy link
Member

Choose a reason for hiding this comment

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

The old API was ok too.

proc pollingMainLoop*(poll: proc(timeout: int); timeout: int) =
export rawui.main
export rawui.mainSteps
export rawui.mainStep
Copy link
Member

Choose a reason for hiding this comment

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

Why? ui encapsulates rawui. If you need rawui, import it yourself.

@@ -140,6 +163,9 @@ proc setChild*(w: Window; child: Widget) =
windowSetChild(w.impl, child.impl)
w.child = child

proc `child=`*(w: Window; c: Widget) =
w.setChild(c)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this alias buys us anything.

Copy link
Author

Choose a reason for hiding this comment

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

consistency

when param2typ is void:
block:
proc generated_handler(phandler: ptr AreaHandler, _: ptr rawui.Area): rettyp {.cdecl.} =
let widget = cast[ptr Area](cast[int](phandler) - offsetOf(AreaObj, handler))
Copy link
Member

Choose a reason for hiding this comment

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

Who knows if this is correct? I don't. Stick to the patterns we already established elsewhere in this codebase for how to wrap callbacks.

Copy link
Author

Choose a reason for hiding this comment

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

This is the proper way. The callback signature for Area is different.

handler.dragBroken = wrapAreaCallback(onDragBroken)
handler.keyEvent = wrapAreaCallback(onKeyEvent, ptr AreaKeyEvent, cint)

a.handler = handler
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
a.handler = handler
a.handler = AreaHandler(
draw: ...,
mouseEvent: ...,
mouseCrossed: ...,
...
)

result.initHandler()
result.impl = rawui.newArea(result.handler.addr)

proc newScrollingArea*(width: cint, height: cint): Area =
Copy link
Member

Choose a reason for hiding this comment

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

The rest of the API exposes procs as taking int and converting it to cint in the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants