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 Qt menu support #2504

Merged
merged 52 commits into from Jun 28, 2018

Conversation

Projects
None yet
7 participants
@scztt
Contributor

scztt commented Nov 17, 2016

This is a preliminary proposal for QT menu support in SCLang. It's built on three core classes - Action, Menu, and Toolbar, which wrap and mirror their QT equivalents (QAction, etc).

In addition, this provides a MainMenu class, which can be used by Quarks or user code to register custom menu items by registering them in a menu and a group using:

    MainMenu.register(Action("Name", { }), "Menu Name");
    MainMenu.register(Action("Other Name", { }), "Menu Name", "Group Name");

CustomViewAction is a subclass of Action that allows embedding any SC widget in a menu or toolbar. Behaviorally, this can be weird (my slider example), but make sense for things like scopes, level meters, or UserViews.

This PR has most or all of the functionality that's required, but I want feedback about the sclang-side APIs, and some good code review before I finalize it. Things requiring some attention are marked with @TODO.

Once there's some agreement on the sclang bits, I'll get some documentation in - in the mean time, this is an example of usage: https://gist.github.com/scztt/c4255e2ccc3fea0214b1d00089bc71ef

The latest build is available for testing here:
http://supercollider.s3.amazonaws.com/builds/supercollider/supercollider/osx/topic/menu-support-latest.html

scztt added some commits Jul 10, 2016

sclang: Generic QObject-descendant TypeCodec
This TypeCodec can be used to convert read and write any QObject-derived class. This allows hooking directly into meta-calls that take QObject-derived types as arguments.

In the read case, it checks to see if the slot contains a QObjectProxy. If so, it attempts to qobject_cast it to the requested type, else it provides a null pointer. Note that read == safeRead: this will always provide a null pointer in cases where the type conversion doesn't work.

In the write case, we simply do the reverse - cast to the expected type, and then convert as a QObject, which will connect with it's QObjectProxy (if it exists).
sclang: Generic TypeCodec for lists
This provides a TypeCodec for e.g. functions that return QList/QVector types.
sclang: Refactor out more QObjectProxy conversions
Now done by generic QObject TypeCodec
sclang: temporary to fix build
This template needs to be fixed?
@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Nov 17, 2016

Contributor

Luca has also had a go at this: https://github.com/LucaDanieli/supercollider/tree/sclang-menu-support

Perhaps worth comparing notes?

Contributor

muellmusik commented Nov 17, 2016

Luca has also had a go at this: https://github.com/LucaDanieli/supercollider/tree/sclang-menu-support

Perhaps worth comparing notes?

@telephon

This comment has been minimized.

Show comment
Hide comment
@telephon

telephon Nov 17, 2016

Member

The latest build you linked doesn't seem to include the Menu and Action classes.

Member

telephon commented Nov 17, 2016

The latest build you linked doesn't seem to include the Menu and Action classes.

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt Nov 17, 2016

Contributor

@telephon In the linked build, i see this:
[I] ~/D/S/SuperCollider.app> find . -iname Menu.sc ./Contents/Resources/SCClassLibrary/Common/GUI/Base/Menu.sc
Is it not there?

@muellmusik Definitely! There was some initial discussion, but I wasn't sure where those things were at currently. LNX_Studio folks have been using this to try to get LNX off of 3.6 / Cocoa SuperCollider, so they've been encouraging me to push the discussion on this branch forward. :)

Contributor

scztt commented Nov 17, 2016

@telephon In the linked build, i see this:
[I] ~/D/S/SuperCollider.app> find . -iname Menu.sc ./Contents/Resources/SCClassLibrary/Common/GUI/Base/Menu.sc
Is it not there?

@muellmusik Definitely! There was some initial discussion, but I wasn't sure where those things were at currently. LNX_Studio folks have been using this to try to get LNX off of 3.6 / Cocoa SuperCollider, so they've been encouraging me to push the discussion on this branch forward. :)

@telephon

This comment has been minimized.

Show comment
Hide comment
@telephon

telephon Nov 17, 2016

Member

Yes, it is there. I realise: it was the language config which links to my devel source files. Hm ...

Member

telephon commented Nov 17, 2016

Yes, it is there. I realise: it was the language config which links to my devel source files. Hm ...

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Nov 18, 2016

Contributor

Luca and I are just looking at this. Two comments:

So the Menu just pops up wherever the mouse pointer is? Is that standard behaviour or is there a need to attach it to a view, or specify position? (e.g. if a menu is opened using the keyboard for example)

Menus don't seem to exactly match native menus (at least on OSX)

Generally, very cool!

Contributor

muellmusik commented Nov 18, 2016

Luca and I are just looking at this. Two comments:

So the Menu just pops up wherever the mouse pointer is? Is that standard behaviour or is there a need to attach it to a view, or specify position? (e.g. if a menu is opened using the keyboard for example)

Menus don't seem to exactly match native menus (at least on OSX)

Generally, very cool!

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt Nov 18, 2016

Contributor

@muellmusik The .front method takes a point and the action that should have initial focus. The point defaults to the mouse pointer, for context menu type behavior - but you can do menu.front(300@200, openAction), which would show it at that point, with whatever openAction is highlighted.

On OSX, the app menus are native. The context menus probably dont match exactly because Qt is generating then itself. If you were using the Macintosh style for Qt (I think you can do QtGUI.style = "macintosh", though not totally sure?) then it may look more native.

Contributor

scztt commented Nov 18, 2016

@muellmusik The .front method takes a point and the action that should have initial focus. The point defaults to the mouse pointer, for context menu type behavior - but you can do menu.front(300@200, openAction), which would show it at that point, with whatever openAction is highlighted.

On OSX, the app menus are native. The context menus probably dont match exactly because Qt is generating then itself. If you were using the Macintosh style for Qt (I think you can do QtGUI.style = "macintosh", though not totally sure?) then it may look more native.

@telephon

This comment has been minimized.

Show comment
Hide comment
@telephon

telephon Nov 18, 2016

Member

This is really nice!

One suggestion so far: maybe QTAction or GuiAction instead of Action?

Member

telephon commented Nov 18, 2016

This is really nice!

One suggestion so far: maybe QTAction or GuiAction instead of Action?

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Nov 19, 2016

Contributor

@muellmusik The .front method takes a point and the action that should have initial focus. The point defaults to the mouse pointer, for context menu type behavior - but you can do menu.front(300@200, openAction), which would show it at that point, with whatever openAction is highlighted.

Okay. The point is screen wide though, yes? That means I still have an issue if I trigger a context menu with the keyboard thus:

(
Button().front.action = {

~menu = Menu(
    Action("A", { "a selected".postln }),
    Action("B", { "b selected".postln }),
    Action("C", { "c selected".postln }),
);

~menu.front; // can be re-invoked after menu is closed
}
)

If I just want to ensure it's attached to the view, I'd have to check its bounds to handle the keyboard case.

On OSX, the app menus are native. The context menus probably dont match exactly because Qt is generating then itself. If you were using the Macintosh style for Qt (I think you can do QtGUI.style = "macintosh", though not totally sure?) then it may look more native.

Yes, that works. I think this should be default. It's a small thing, but mildly distracting.

Contributor

muellmusik commented Nov 19, 2016

@muellmusik The .front method takes a point and the action that should have initial focus. The point defaults to the mouse pointer, for context menu type behavior - but you can do menu.front(300@200, openAction), which would show it at that point, with whatever openAction is highlighted.

Okay. The point is screen wide though, yes? That means I still have an issue if I trigger a context menu with the keyboard thus:

(
Button().front.action = {

~menu = Menu(
    Action("A", { "a selected".postln }),
    Action("B", { "b selected".postln }),
    Action("C", { "c selected".postln }),
);

~menu.front; // can be re-invoked after menu is closed
}
)

If I just want to ensure it's attached to the view, I'd have to check its bounds to handle the keyboard case.

On OSX, the app menus are native. The context menus probably dont match exactly because Qt is generating then itself. If you were using the Macintosh style for Qt (I think you can do QtGUI.style = "macintosh", though not totally sure?) then it may look more native.

Yes, that works. I think this should be default. It's a small thing, but mildly distracting.

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt Nov 20, 2016

Contributor

@telephon I worried about taking such a general noun, Action, in the namespace. I avoided things like QTAction or GuiAction because we don't have any similar prefixes for GUI classes. I would maybe suggest MenuAction? I don't think this is entirely right though - Action's in QT have wider application than just menus. For example, we would gain a lot by allowing use of Actions for Button:action, since Actions track en/disablement, title strings, tooltip strings, shortcut keys in a way that's abstracted from the widget itself. The Server class might have several actions (boot, quit, kill, free all), that could be connected to both buttons and menu items, and all would share enablement, titles, shortcut keys, etc.

Contributor

scztt commented Nov 20, 2016

@telephon I worried about taking such a general noun, Action, in the namespace. I avoided things like QTAction or GuiAction because we don't have any similar prefixes for GUI classes. I would maybe suggest MenuAction? I don't think this is entirely right though - Action's in QT have wider application than just menus. For example, we would gain a lot by allowing use of Actions for Button:action, since Actions track en/disablement, title strings, tooltip strings, shortcut keys in a way that's abstracted from the widget itself. The Server class might have several actions (boot, quit, kill, free all), that could be connected to both buttons and menu items, and all would share enablement, titles, shortcut keys, etc.

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt Nov 20, 2016

Contributor

@muellmusik Good point - if you want to show a menu that's a "popup" from another view, obviously a mouse pos isn't the right place. In your case, this would need to be something like:

Button().front.action = {
    |b|
    ~menu = Menu(
        Action("A", { "a selected".postln }),
        Action("B", { "b selected".postln }),
        Action("C", { "c selected".postln }),
    );

    ~menu.front(b.bounds.leftBottom); // can be re-invoked after menu is closed
}
)

Should I add a method like View:showPopupMenu that does the positioning automatically perhaps?

Contributor

scztt commented Nov 20, 2016

@muellmusik Good point - if you want to show a menu that's a "popup" from another view, obviously a mouse pos isn't the right place. In your case, this would need to be something like:

Button().front.action = {
    |b|
    ~menu = Menu(
        Action("A", { "a selected".postln }),
        Action("B", { "b selected".postln }),
        Action("C", { "c selected".postln }),
    );

    ~menu.front(b.bounds.leftBottom); // can be re-invoked after menu is closed
}
)

Should I add a method like View:showPopupMenu that does the positioning automatically perhaps?

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Nov 21, 2016

Contributor

Should I add a method like View:showPopupMenu that does the positioning automatically perhaps?

Yes, there should be an easy way to make sure that's correct. Not sure of the best syntax, but that seems reasonable.

I avoided things like QTAction or GuiAction because we don't have any similar prefixes for GUI classes.

Definitely not QTAction or anything that ties to the implementation. GUIAction seems fine to me though, since it's basically a label and a function that could be interpreted in various ways.

Contributor

muellmusik commented Nov 21, 2016

Should I add a method like View:showPopupMenu that does the positioning automatically perhaps?

Yes, there should be an easy way to make sure that's correct. Not sure of the best syntax, but that seems reasonable.

I avoided things like QTAction or GuiAction because we don't have any similar prefixes for GUI classes.

Definitely not QTAction or anything that ties to the implementation. GUIAction seems fine to me though, since it's basically a label and a function that could be interpreted in various ways.

@telephon

This comment has been minimized.

Show comment
Hide comment
@telephon

telephon Nov 21, 2016

Member

Yes, GUIAction or ViewAction makes sense. In this case, I'd consider "GUI" or "View" not as a namespace indicator, but simply as an adjective that replies to "which action?".

Member

telephon commented Nov 21, 2016

Yes, GUIAction or ViewAction makes sense. In this case, I'd consider "GUI" or "View" not as a namespace indicator, but simply as an adjective that replies to "which action?".

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt Nov 22, 2016

Contributor

I like ViewAction a little less, but it's easier to remember and better matches existing naming conventions, so I'll go with that.

Contributor

scztt commented Nov 22, 2016

I like ViewAction a little less, but it's easier to remember and better matches existing naming conventions, so I'll go with that.

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Dec 18, 2016

Contributor

So where are we with this? Would be nice to see this finalised. Certainly let's not let it drift.

Contributor

muellmusik commented Dec 18, 2016

So where are we with this? Would be nice to see this finalised. Certainly let's not let it drift.

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt Dec 22, 2016

Contributor

I'll do a little clean-up around the things discussed, and then I think we should merge. We can keep iterating, but the core sclang API's feel about right to me, and that's the important part. It would be helpful to get an assist on the help files - is it acceptable to check in with just the important things documented, and fill in with examples etc as we move forward? Or should we try to nail the help 100% before merging?

Contributor

scztt commented Dec 22, 2016

I'll do a little clean-up around the things discussed, and then I think we should merge. We can keep iterating, but the core sclang API's feel about right to me, and that's the important part. It would be helpful to get an assist on the help files - is it acceptable to check in with just the important things documented, and fill in with examples etc as we move forward? Or should we try to nail the help 100% before merging?

@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Dec 22, 2016

Member

I'd be happy to help with the documentation, if there's anything I can do.

Member

brianlheim commented Dec 22, 2016

I'd be happy to help with the documentation, if there's anything I can do.

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Dec 22, 2016

Contributor

is it acceptable to check in with just the important things documented, and fill in with examples etc as we move forward? Or should we try to nail the help 100% before merging?

I think that's okay, as long as it's documented. What we don't want is releases going out with undocumented stuff. There's far too much of that in SC.

Contributor

muellmusik commented Dec 22, 2016

is it acceptable to check in with just the important things documented, and fill in with examples etc as we move forward? Or should we try to nail the help 100% before merging?

I think that's okay, as long as it's documented. What we don't want is releases going out with undocumented stuff. There's far too much of that in SC.

@snappizz

This comment has been minimized.

Show comment
Hide comment
@snappizz

snappizz Jan 14, 2017

Member

How are we doing here?

Member

snappizz commented Jan 14, 2017

How are we doing here?

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt Jun 19, 2018

Contributor
  • Status tip is just a tooltip - added help, I missed that one.
  • You're right re. asAction - updated.
  • asAction and asMenu are only used internally, to allow passing of View's and Strings in place of MenuActions when constructing a menu:
Menu(
	"Menus",
	s.scope.view.asView,
	"are",
	Slider().orientation_(\horizontal),
	"fun."
).front;```

Contributor

scztt commented Jun 19, 2018

  • Status tip is just a tooltip - added help, I missed that one.
  • You're right re. asAction - updated.
  • asAction and asMenu are only used internally, to allow passing of View's and Strings in place of MenuActions when constructing a menu:
Menu(
	"Menus",
	s.scope.view.asView,
	"are",
	Slider().orientation_(\horizontal),
	"fun."
).front;```

@telephon

This comment has been minimized.

Show comment
Hide comment
@telephon

telephon Jun 19, 2018

Member

if these messages are only used internally, it would be better to give them a more specific name. This would avoid filling the namespace for "public" messages.

Member

telephon commented Jun 19, 2018

if these messages are only used internally, it would be better to give them a more specific name. This would avoid filling the namespace for "public" messages.

@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Jun 19, 2018

Member

Status tip is just a tooltip - added help, I missed that one.

Isn't that what tooltip is? I think status tip is something different: https://doc.qt.io/qt-5/qaction.html#statusTip-prop

You're right re. asAction - updated.
asAction and asMenu are only used internally, to allow passing of View's and Strings in place of MenuActions when constructing a menu:

Partially re: @telephon above, does "internal" here mean private or just not useful for most applications?

Member

brianlheim commented Jun 19, 2018

Status tip is just a tooltip - added help, I missed that one.

Isn't that what tooltip is? I think status tip is something different: https://doc.qt.io/qt-5/qaction.html#statusTip-prop

You're right re. asAction - updated.
asAction and asMenu are only used internally, to allow passing of View's and Strings in place of MenuActions when constructing a menu:

Partially re: @telephon above, does "internal" here mean private or just not useful for most applications?

@telephon

This comment has been minimized.

Show comment
Hide comment
@telephon

telephon Jun 19, 2018

Member

Partially re: @telephon above, does "internal" here mean private or just not useful for most applications?

I suppose that these methods could be implemented by any object you like to be used within this view system. So they are not completely internal. I am perfectly fine with asMenuAction !

(if they were really private, we could name them prSomething to make this clear to the reader, but I don't think this is the intention, because they are calls to other objects).

Member

telephon commented Jun 19, 2018

Partially re: @telephon above, does "internal" here mean private or just not useful for most applications?

I suppose that these methods could be implemented by any object you like to be used within this view system. So they are not completely internal. I am perfectly fine with asMenuAction !

(if they were really private, we could name them prSomething to make this clear to the reader, but I don't think this is the intention, because they are calls to other objects).

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt Jun 19, 2018

Contributor

asMenuAction and asMenu match the pattern used across sclang for use-this-thing-as-this-other-thing poloymorphism (asView, asLayout are similar examples for UI).

Contributor

scztt commented Jun 19, 2018

asMenuAction and asMenu match the pattern used across sclang for use-this-thing-as-this-other-thing poloymorphism (asView, asLayout are similar examples for UI).

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt Jun 19, 2018

Contributor

Yes, after looking a little more at statusTip, it's semantically similar to a tooltip, but only shown in status bars (which we currently can't show in sclang). That aside, if we had status tips, they should be implemented in the base view class and not menu anyway. I'll remove this?

Contributor

scztt commented Jun 19, 2018

Yes, after looking a little more at statusTip, it's semantically similar to a tooltip, but only shown in status bars (which we currently can't show in sclang). That aside, if we had status tips, they should be implemented in the base view class and not menu anyway. I'll remove this?

@orbsmiv

This comment has been minimized.

Show comment
Hide comment
@orbsmiv

orbsmiv Jun 20, 2018

Contributor

It seems as though ListView may be broken on this topic branch (commit 8b41eb5) – items in the list can be visually selected but the action method isn't being called. I've compared to commit 60919df on the develop branch, which is working as expected.

Contributor

orbsmiv commented Jun 20, 2018

It seems as though ListView may be broken on this topic branch (commit 8b41eb5) – items in the list can be visually selected but the action method isn't being called. I've compared to commit 60919df on the develop branch, which is working as expected.

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt Jun 21, 2018

Contributor

@orbsmiv I don't think what you described is related to this branch - there are no changes to ListView at all. It may be related to the recent QT upgrade - note that selectionAction and enterKeyAction both work for ListView, probably action needs to be hooked up to the same thing as selectionAction, at least if that's the correct behavior.
Can you file a bug on this?

Contributor

scztt commented Jun 21, 2018

@orbsmiv I don't think what you described is related to this branch - there are no changes to ListView at all. It may be related to the recent QT upgrade - note that selectionAction and enterKeyAction both work for ListView, probably action needs to be hooked up to the same thing as selectionAction, at least if that's the correct behavior.
Can you file a bug on this?

@orbsmiv

This comment has been minimized.

Show comment
Hide comment
@orbsmiv

orbsmiv Jun 21, 2018

Contributor

@scztt I thought so too but as a test I built from the latest commit on the develop branch, which I believe has all of the new webengine stuff (is this the QT upgrade you're referring to?), using QT5.11.1 and ListView worked fine. I wondered whether the changes to QcListWidget.cpp in commit 2ce4c4d might be the cause? If I'm barking up the wrong tree then I'll happily file a bug report!

Contributor

orbsmiv commented Jun 21, 2018

@scztt I thought so too but as a test I built from the latest commit on the develop branch, which I believe has all of the new webengine stuff (is this the QT upgrade you're referring to?), using QT5.11.1 and ListView worked fine. I wondered whether the changes to QcListWidget.cpp in commit 2ce4c4d might be the cause? If I'm barking up the wrong tree then I'll happily file a bug report!

brianlheim added some commits Jun 21, 2018

qtgui: fix message connections
qctreewidget and qclistwidget failed to send 'currentItemChanged' messages due to a previous commit (2ce4c4d)
qtgui: Menu: remove statusTip[_]
this functionality is unimplemented and we have no plan to support it
@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Jun 21, 2018

Member

asMenuAction and asMenu match the pattern used across sclang for use-this-thing-as-this-other-thing poloymorphism (asView, asLayout are similar examples for UI).

This is fine.

Yes, after looking a little more at statusTip, it's semantically similar to a tooltip, but only shown in status bars (which we currently can't show in sclang). That aside, if we had status tips, they should be implemented in the base view class and not menu anyway. I'll remove this?

I've removed it in 325ec83, hope you don't mind.

It seems as though ListView may be broken on this topic branch (commit 8b41eb5) – items in the list can be visually selected but the action method isn't being called. I've compared to commit 60919df on the develop branch, which is working as expected.

I tested this and both ListView and TreeView were broken by 2ce4c4d. The last commit (1872682) fixes this.

Member

brianlheim commented Jun 21, 2018

asMenuAction and asMenu match the pattern used across sclang for use-this-thing-as-this-other-thing poloymorphism (asView, asLayout are similar examples for UI).

This is fine.

Yes, after looking a little more at statusTip, it's semantically similar to a tooltip, but only shown in status bars (which we currently can't show in sclang). That aside, if we had status tips, they should be implemented in the base view class and not menu anyway. I'll remove this?

I've removed it in 325ec83, hope you don't mind.

It seems as though ListView may be broken on this topic branch (commit 8b41eb5) – items in the list can be visually selected but the action method isn't being called. I've compared to commit 60919df on the develop branch, which is working as expected.

I tested this and both ListView and TreeView were broken by 2ce4c4d. The last commit (1872682) fixes this.

@brianlheim

All my concerns have been addressed!

@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Jun 23, 2018

Member

@telephon - can you please update/dismiss your review? I believe this is ready for merge

Member

brianlheim commented Jun 23, 2018

@telephon - can you please update/dismiss your review? I believe this is ready for merge

@telephon

This comment has been minimized.

Show comment
Hide comment
@telephon

telephon Jun 23, 2018

Member

the example still has a addDependant without a removeDependant.

Member

telephon commented Jun 23, 2018

the example still has a addDependant without a removeDependant.

@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Jun 23, 2018

Member

the example still has a addDependant without a removeDependant.

Why is that a problem?

Member

brianlheim commented Jun 23, 2018

the example still has a addDependant without a removeDependant.

Why is that a problem?

@telephon

This comment has been minimized.

Show comment
Hide comment
@telephon

telephon Jun 23, 2018

Member

Well it's not a good example of how to do things because it creates a (small) memory leak. You will have the object dangling in the dependents dictionary that will never be garbage collected.

// from Object:
addDependant { arg dependant;
		var theDependants;
		theDependants = dependantsDictionary.at(this);
		if(theDependants.isNil,{
			theDependants = IdentitySet.new.add(dependant);
			dependantsDictionary.put(this, theDependants);
		},{
			theDependants.add(dependant);
		});
	}
Member

telephon commented Jun 23, 2018

Well it's not a good example of how to do things because it creates a (small) memory leak. You will have the object dangling in the dependents dictionary that will never be garbage collected.

// from Object:
addDependant { arg dependant;
		var theDependants;
		theDependants = dependantsDictionary.at(this);
		if(theDependants.isNil,{
			theDependants = IdentitySet.new.add(dependant);
			dependantsDictionary.put(this, theDependants);
		},{
			theDependants.add(dependant);
		});
	}
docs: fix examples using addDependant
make sure to demonstrate correct cleanup usage
@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Jun 24, 2018

Member

@telephon Thanks for the explanation! I fixed the examples and made sure that the dependants are removed from the global dictionary after the windows and menus are closed. How's it look now?

Member

brianlheim commented Jun 24, 2018

@telephon Thanks for the explanation! I fixed the examples and made sure that the dependants are removed from the global dictionary after the windows and menus are closed. How's it look now?

@snappizz

This comment has been minimized.

Show comment
Hide comment
@snappizz

snappizz Jun 24, 2018

Member

i'm a little late to the party here, but could we do Toolbar instead of ToolBar? everywhere i've seen spells it as one word

Member

snappizz commented Jun 24, 2018

i'm a little late to the party here, but could we do Toolbar instead of ToolBar? everywhere i've seen spells it as one word

@snappizz snappizz changed the title from Topic/menu support to Add Qt menu support Jun 24, 2018

@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Jun 24, 2018

Member

The corresponding Qt class is QToolBar. I think it's fine as is.

Member

brianlheim commented Jun 24, 2018

The corresponding Qt class is QToolBar. I think it's fine as is.

@snappizz

This comment has been minimized.

Show comment
Hide comment
@snappizz

snappizz Jun 24, 2018

Member

is MainMenu supported on non-mac platforms? sclang doesn't have an application menu (at least on ubuntu + xfce), not sure about windows

also, this is tagged "API change" -- how so?

Member

snappizz commented Jun 24, 2018

is MainMenu supported on non-mac platforms? sclang doesn't have an application menu (at least on ubuntu + xfce), not sure about windows

also, this is tagged "API change" -- how so?

@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Jun 24, 2018

Member

I added a note that MainMenu is only available on Mac OS.

Member

brianlheim commented Jun 24, 2018

I added a note that MainMenu is only available on Mac OS.

@snappizz

thanks all! will merge once CI is one!

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt Jun 25, 2018

Contributor

The menu bar should be available on some Linux variants (I think Ubuntu?) - basically anywhere that has a menu bar that is not attached to a specific window.

Contributor

scztt commented Jun 25, 2018

The menu bar should be available on some Linux variants (I think Ubuntu?) - basically anywhere that has a menu bar that is not attached to a specific window.

@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Jun 25, 2018

Member

The AppVeyor failures are purely due to storage limit exceeded (I've put in a request that hasn't been addressed yet because it's Sunday night).

Member

brianlheim commented Jun 25, 2018

The AppVeyor failures are purely due to storage limit exceeded (I've put in a request that hasn't been addressed yet because it's Sunday night).

@brianlheim brianlheim merged commit 52a0296 into develop Jun 28, 2018

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@brianlheim brianlheim deleted the topic/menu-support branch Jun 28, 2018

@snappizz

This comment has been minimized.

Show comment
Hide comment
@snappizz

snappizz Jun 28, 2018

Member

🎉

Member

snappizz commented Jun 28, 2018

🎉

brianlheim added a commit to brianlheim/supercollider that referenced this pull request Jun 29, 2018

treeview: fix setItemWidget
#2504 introduced changes to the metatype system that made the type of this method inconsistent with what Qt GUI expected. this fixes a method-not-found error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment