search

sghpjuikit / player

Audio player and management application.
22 stars 2 forks source link

Refactor CoreMenus #73

Closed xeruf closed 5 years ago

xeruf commented 6 years ago

WIP - I want to avoid the verbosity and repetitions of the previous syntax and add some items

sghpjuikit commented 6 years ago

What is the status of this PR?

xeruf commented 6 years ago

Still broken, but I'll work on it when I have time.

xeruf commented 5 years ago

Current problem: MenuItems with subitems do not show up. Everything else seems fine.

xeruf commented 5 years ago

Got that, was a dumb mistake. The "File" menu with its submenus still fails to show up though.

sghpjuikit commented 5 years ago

Do you want me to land a hand? I do have other things to work on, but if you need me to check the code I do not mind.

xeruf commented 5 years ago

Yep, I have pushed the changes and added a TODO at the relevant position. How can I test the addMany for files though? I am not able to select multiple files in the DirViewer.

sghpjuikit commented 5 years ago

It's been a while since I did anything relevant in the class so I'm out of loop and cant even reply to you without taking a look at your code. Sorry. I will, once I do that.

xeruf commented 5 years ago

You do not have to go into the code for that. I just want to know in which widget I can select multiple files at once and invoke the contextmenu on them.

xeruf commented 5 years ago

Okay found it, again a dumb mistake. Once I push the fix this should be ready for merge.

sghpjuikit commented 5 years ago

I'll take a look once I push my local changes.

sghpjuikit commented 5 years ago

I played around with the menus and discovered multiple problems with the design (not necessarily caused with this PR). Quite important.

1 Moving the ContextMenuBuilder.item()/menu() out as an extension function causes conflict with the Menu.menu()/item() extensions. In fact the menu is not populated with items as of now, because wrong extension is imported in CoreMenu. The correct solution is to unify these extensions. i.e., remove Menu.menu()/item() from utils and move ContextMenuBuilder.item()/menu() to utils instead. It did not occur to me, but this should have been done at the very beginning. I temporarily fixed it by making item() member function instead of extension.

2 There is a problem with addSingle/addMany. The menu generation works that either 'single' or 'many' items suppliers are used, never both at once. Unfortunately that creates duplication. Because one can not reuse items from List for File. But what happens when the value is List with single file? It should be unpacked of course, but that will hide the actions for List, which does not truly make sense. I have fixed this so both suppliers are used, i.e., if the value is File, it will also show menu items for type List,. This makes sense because the actions are applicable. Unfortunately, this causes order problem, where developer loses control over the order of the items. The items for List will always be after those for File, but this is not always desired. The only solution is to merge addSingle() and addMultiple() into single function. Not easy and I would postpone this into the future as UX improvement.

3 There was a bug in unpacking singleton List into T resulting in ClassCastException. Not sure how it escaped detection until now. I fixed it by implicit collectionWrap/Unwrap when consuming the value.

4 We ignored separators. Those are MenuItems too. API needs to be extended. After issue 1 is fixed. I only added a factory for now.

5 Menu creation and blocking. Blocking ui is no go. I'm thinking about moving the menu construction into bgr thread where we will be able to do even I/O. Fortunately javaFX does not forbid creation of its objects in bgr. Should be doable.

5 Testing. I did multiple fixes/improvements to be able to test/leverage the menus.

xeruf commented 5 years ago

You are again putting some Inspector and util changes in here, which have nothing to do with this PR. Could you please just approve this so I can merge and prevent you from being tempted to add more unrelated changes?

sghpjuikit commented 5 years ago

No I can't. It did not even work without reverting the extension method. The namespace clash is serious problem that needs to be addressed. I wrote about these problems above. 1 and 4 must be addressed before accepting the merge. I can fix these, if you do want me to.

The changes I made were related, specifically for making it possible to test the menus better. You asked multiple times how to test addMany(). Well by drag & dropping files onto Inspector, that's how.

sghpjuikit commented 5 years ago

Fixed 1 by adjusting imports. Turns out the namespace does not clash because extension methods are resolved correctly due to receiver type priority, but only if both 'clashing' methods are imported. This is a little magical and difficult to spot but I'm leaving it as is as there is no easy way around it.

2 Isn't that big of a problem. Will stay as is.

3 Was still confusing. I ended up Removing ValueContextMenu altogether. Much better now.

Implemented 4.

xeruf commented 5 years ago

You could have committed the changes to Inspector directly to master and then merged master back in.

1) I do not think the ContextMenuBuilder should have extension methods in util, unless you move the actual class to utils as well, which doesn't seem like a bad idea tbh.

2) "I have fixed this so both suppliers are used, i.e., if the value is File, it will also show menu items for type List,. This makes sense because the actions are applicable." I do not really agree with this. Firstly, you spell the actions differently depending on whether it is one or multiple items, and secondly, there are actions that only make sense on multiple items.

sghpjuikit commented 5 years ago

I fixed more problems related to ImprovedContextMenu

The merge is now ready, pls check if you are satisfied with all the solutions.

"I do not think the ContextMenuBuilder should have extension methods in util" You are right. It was a bad idea. Final solution was to just import both item extensions and let the compiler apply each in correct context.

"Firstly, you spell the actions differently depending on whether it is one or multiple items, and secondly, there are actions that only make sense on multiple items." The former can be avoided with smart wording or solved using String.pluralize(count: Int) extension The latter may be true. Do you have any example?

We can revert this concept, but I think it makes more sense than it doesn't. Consider File.browse, which also has List.browse, which already handles the single file case. Having separate menus with different implementations may lead to unexpected behaviors. Also, any T is conceptually List. If an action only makes sense for list.size>1 then it is the action's responsibility to add the proper test. I think.

xeruf commented 5 years ago

you gotta push them for me to have a look ^^

sghpjuikit commented 5 years ago

The changes to Inspector were done because of this branch. That means they belong to this branch right?

sghpjuikit commented 5 years ago

Although this branch seems to have stalled/bloated, I actually think it was simply result of iterative development and that there were great improvements compared to what master is doing. This PR taking its time is something I dont find disadvantageous at all.

sghpjuikit commented 5 years ago

Removed

sghpjuikit commented 5 years ago

I'm done, you can proceed. I suggest no squashing.

xeruf commented 5 years ago

This branch has 21 commits, easily half of them being corrections and merges. Are you sure you want to have them all in master?

xeruf commented 5 years ago

Also, you still need to approve for me to merge.

sghpjuikit commented 5 years ago

I'm not big on 'clean' history. Im 50:50, your call.

sghpjuikit commented 5 years ago

Id appreciate if you merged.

xeruf commented 5 years ago

Right, I forgot. Will do today.