search

elieserdejesus / JamTaba

Jamtaba is a software to play online music jam sessions.
http://www.jamtaba.com
245 stars 49 forks source link

GUI do not respond when launching Jamtaba in full screen mode and then exiting full screen mode #276

Closed jonjamcam closed 8 years ago

jonjamcam commented 8 years ago

It works OK if you return to full screen mode again. So to test the bug:

1.- launch Jamtaba and set full screen mode 2.- close Jamtaba and launch again 3.- change to normal mode 4.- GUI not responsive. 5.- Go to full screen mode again 6.- GUI now OK

elieserdejesus commented 8 years ago

@jonjamcam , I can't reproduce this bug here (using the JT 2.0.13). How you detected the non responsive GUI?

jonjamcam commented 8 years ago

@elieserdejesus OK my bad explanation.

So to test the bug:

1.- launch Jamtaba and set full screen mode 2.- close Jamtaba and launch again 3.- change to normal mode USING F11 (if you use the menu option, the bug is not always triggered) 4.- GUI not responsive. 5.- Go to full screen mode again 6.- GUI now OK

elieserdejesus commented 8 years ago

Jon, I still can't reproduce the bug. These are my steps: 1 - Launching JT and setting to full screen (tested using the menu and F11) 2 - closing and launching again 3 - pressing F11

no bug here, I can use the buttons, listen the rooms, etc.

jonjamcam commented 8 years ago

@elieserdejesus Please try using only F11 to change views:

1.- launch Jamtaba and set full screen mode USING F11 2.- close Jamtaba and launch again 3.- change to normal mode USING F11 4.- GUI not responsive. 5.- Go to full screen mode again 6.- GUI now OK

Just tested and I got the bug. @teamikl can you test this bug please? Maybe it's local in my computer only.

elieserdejesus commented 8 years ago

@jonjamcam and @teamikl , I really can't reproduce the bug. My steps with more details:

0 - I'm using the JT 2.0.13 to test, the official released version installed in my machine. I'm not running from Qt Creator. 1 - Launching JT in full view (not mini node) and setting to full screen (F11) 2 - closing (using ESC) and launching again 3 - backing to normal mode pressing F11

no bug, working fine.

jonjamcam commented 8 years ago

@elieserdejesus ok. I will try tomorrow to test in another laptop with win10.

teamikl commented 8 years ago

not happen to me I tested official release 2.0.13 win64 standalone only.

which version @jonjamcam tested ? VST ? about they press key action I think is VST had keyboard hook code.

BTW, ESC quit program!? I know this behavior was same in Ninjam standalone client, and I wanted this to improve. Japanese users use ESC key for cannel typing in IME (input method)

this trouble happen many time, when chatting in Japanese word quit the program by mistake. I will make new issue later listen other japanese user's opinion and feedback.

jonjamcam commented 8 years ago

which version @jonjamcam tested ?

It's happening with Standalone 2.0.13 ATM.

BTW, ESC quit program!?

Yes. Key shortcuts work, but the mouse is useless. Later I will have a win10 laptop to test. I will post a video too showing what's the problem in my computer, but probably it's just me :(.

EDIT: compiled 2.0.14 has the same problem.

jonjamcam commented 8 years ago

@elieserdejesus , @teamikl Just tested in my win7 and another win10 laptop and the bug appears in both.

I use only ESC to exit and F11 to set full screen on/off.

EDIT: I uploaded a little video so you can see:

https://youtu.be/iRIbGMDlpnY

elieserdejesus commented 8 years ago

@jonjamcam , you tested in 2 win 7? If yes, are 64 or 32 bits? I will try test in a virtual machine ...

jonjamcam commented 8 years ago

@elieserdejesus The video was a win7 test. The other laptop is win10, both are 64bit

elieserdejesus commented 8 years ago

@jonjamcam , I watch your video and see the totally freezed GUI. I will try reproduce your steps exactly here. For example, you are launching the .exe manually, I'm using the icon in desktop. You are listening the room before full screen, I'm not. These are little details, but who knows? :D I will test again soon.

teamikl commented 8 years ago

@jonjamcam another difference for me, when you launch the exe file, what dialog is shown? it was not happen for me. is it run as admin?

if it's happen specific case only then doubt environments.

and this is my case

btw, http://www.cockos.com/licecap/ is useful for make such simple animated screen captures. (NOTE maybe prev version work for win7)

capture-demo

@elieserdejesus about freeze GUI case, just share info programming perspective no responsive GUI it's often deadlock, busy function which not return to qt event loop. hard to see all cases from this view points.

about view mode management, "state" often make this situation, it happen when program was specific state, because a lot if stateCheck() codes.

Qt provides state machine framework, (you can image observer pattern) that can write code property changes automatically by state change, that can remove isXXXState() code everywhere. (this will require code changes widely, so this's not request to change, just stay as info)

jonjamcam commented 8 years ago

another difference for me, when you launch the exe file, what dialog is shown?

It's a win7 protection saying that the program wants to do changes in the system and asks if I approve or not. This appears every time a x64 app wants to run. Win10 does not show the warning.

audio devices via ASIO4ALL?

@teamikl Yes, both Laptops use ASIO4ALL.

application config

I always do clean install. Both tests were done with clean install.

NOTE: I tested the 2.0.14 recently compiled using remote branch: 'translating-country-names' and the bug DOES NOT happen.

elieserdejesus commented 8 years ago

@teamikl , about the State machine, I like this idea, it's very clean. But I'm no shure if all the things we need change are "properties" in a Qt sense. Off course we can promote some attributes to properties, but this is a medium/big refactoring task. Let's put the idea in the pile :D

@jonjamcam

NOTE: I tested the 2.0.14 recently compiled using remote branch: 'translating-country-names' and the bug DOES NOT happen.

Now we have 2 problems: the bug, and the bug missing :D I changed somethings in the mainWindow in the translating-country-names branch, but nothing directly related with full screen. So at moment I have no idea why the bug is missing in this branch.

jonjamcam commented 8 years ago

@elieserdejesus , @teamikl

Ok. I think I found the cause of the problem.

I deleted the Jamtaba64.json file and the bug disappeared. This is how Jamtaba looks when launched with no .json file:

image

So there's no problem in this case. The bug is Gone for now.

Now When I close Jamtaba a Jamtaba64.json file is created:

Jamtaba64.json.txt

We launch Jamtaba again and there's still no problem.

Now here's the catch. My screen is too small to show all Jamtaba controls, so I maximize. Still no problem.

Now I close Jamtaba, launch again and restore the normal window ( not maximized). Please look how the screen went all the way down:

image

This is where I think the bug is created. To test I close Jamtaba and save the .json file at this stage:

Jamtaba64.json.txt

Now I test. Launch Jamtaba, maximize, press F11, close, F11 again. Now we have the bug.

teamikl commented 8 years ago

@elieserdejesus about the state machine framework, I think the same too refactoring this is unnecessary task or low priority. new code when add state, can consider use. it's useful when you want to add animation.

@jonjamcam it's infomative! we will figure out the problem.

I am not sure how to test your config file, that may not fit to my environment, I wonder the audioDevice value. so I am not testing.

@elieserdejesus here is the diff, green line is first file. red line is the second, problem case.


124c124
<         "audioDevice": 3462088,
---
>         "audioDevice": 4117448,
126c126
<         "firstIn": 2553072,
---
>         "firstIn": 2094224,
128c128
<         "lastIn": 1077515478,
---
>         "lastIn": 1067554006,
153c153
<     "intervalProgressShape": 3407874,
---
>     "intervalProgressShape": 4063234,
166c166
<         "recordingPath": "C:/Users/jon/Documents/Jamtaba"
---
>         "recordingPath": ""
169c169
<     "translation": "es",
---
>     "translation": "",
176,177c176,177
<             "x": 0.069546118378639221,
<             "y": 0.15234375
---
>             "x": 0,
>             "y": 0

Now rest topics, I think improvements for that case.

I think this kind problem could happen only for debug/testers. Users who use only release build may not meet the problem.

about clean installation I tried uninstaller it confirmed remove settings. so, use Uninstaller and remove all settings is also a solution.

elieserdejesus commented 8 years ago

@jonjamcam , I tried to reproduce your steps here, but no bug again.

@teamikl , the first values in the config value are memory garbage values. The audioDevice, for example, is the index of the ASIO device. If Jon is using only ASIO4ALL the value will be 0 (first index). Possibily the correct values are stored when Jamtaba is closed.

My first suspect is from the window position values: x and y. These values are representing the window position, but translated to [0, 1]. For example, if the window left border is in the center of screen the value of x will be 0.5. The numbers in Jon .json files are ok, these are valid values. No clues at moment.

jonjamcam commented 8 years ago

My first suspect is from the window position values: x and y

I did some tests and forced the x and y values to 0, 0.1 and 0.01 after the .json file changes them to

"x": 0.069546118378639221, "y": 0.15234375

This to match the original .json file without the bug. It didn't work. At first the screen appears in the correct position, but after restart, it's again in that low position and the bug is triggered. The json file shows again this long values even hough I had changed them to 0, etc. The question is why does the .json file changes this values to specifically this values all the time?

My theory is that the full screen mode forces this values, but I don't have a way to visualize it. Just a guess.

NOTE: The bug is triggered when the screen moves down like here:

No bug is triggered when I manage to get the position from the top:

teamikl commented 8 years ago

@jonjamcam It reproduced for me now. I don't remember what I changed, after clean installation it happens again.

@elieserdejesus autioDevice was index value!? if user had some devices, and different device was available then the value may be incorrect.

elieserdejesus commented 8 years ago

The question is why does the .json file changes this values to specifically this values all the time?

Maybe this is the problem. I think the json is saved in every main window position change (I don't check the code yet). If this is true, It's a bad approach, save the position when Jamtaba is closed is enough.

autioDevice was index value!? if user had some devices, and different device was available then the value may be incorrect.

Yes Tea, If you are using one device (ASIO4All, for example), close Jamtaba, plug a second ASIO device and this device appears first in the PortAudio devices list this new device will be picked by Jamtaba.

Why hell use this approach, right? hehe. In Reaper, for example, when I plug my M-Audio Fast Track Ultra, close Reaper, unplug my fast track and re open Reaper, the "Preferences" dialog explode in my face showing some audio device error, and the "Fast track" is selected as the audio device. So I ask my self: Why hell this software is trying to use my unplugged audio device? Why not just select the only available device (ASIO4ALL) in this case?

Maybe I take a wrong approach, but use the device index make sense for me, use the device name is more problematic.

teamikl commented 8 years ago

@elieserdejesus about device index I think that same happen for others too, if user selected the last indexed device. and about channel? each devices may have different numbers of input channels.

how about record primary and secondary devices? the second used as fallback.

elieserdejesus commented 8 years ago

@teamikl

and about channel? each devices may have different numbers of input channels.

The channels are always sanitized. For example, if you are using the channels 3 & 4 (the second stereo pair) for a multi-channel audio interface, close Jamtaba, unplug your USB interface and restart JT using ASIO4ALL (assuming 2 channels only) the channels 3 & 4 will be translated to 1 & 2 (the first stereo pair). The stereo will be preserved, but off course is not possible use 4 channels if ASIO4ALL is running in a 2 channels device. If is not possible use stereo (just one input channel selected in ASIO4ALL config) JT will fallback to mono. I spend a lot of time working in these scenarios because change the audio device is very common. Using presets, for example, without channels sanitizing is impossible. You saved a preset using a 8 channels interface, turn off and restart using ASIO4ALL, the presets are off course using invalid channels.

how about record primary and secondary devices? the second used as fallback.

Tea, I think is too much. I'm assuming the common scenario for Jamtaba users is: people take a USB audio interface, plug and play in Jamtaba. Eventually (rare cases) people will plug new USB interfaces. Adding a "secondary device" feature we need something in the preferences dialog to "check/uncheck" this secondary device. And possibly another check/uncheck for the primary device. This will add more complexity in the GUI for common users. When I say "Common users" I'm thinking in "people using just one ASIO device or ASIO4ALL + an external audio device".

Every time I see a suggestion for "add something in the GUI" my first question is: how many people will benefit from this? Or in another words: what is the impact in the common user experience? In this case, if we add primary/secondary device selectors in the GUI we are (in my opnion) adding more complexity for a very small group of musicians.

Sorry if I'm "sounding" too pedantic, but is difficult disagree with good ideas. I'm just trying show (using my very limited english) why this good idea can be not good for common users (too much complexity in GUI). And, off course, I never asked users about this, so it's just my opinion, I don't have real "facts" about common users devices or preferences.

teamikl commented 8 years ago

@elieserdejesus I understand your point it's target common cases. It make sense, should be designed so first.

about reaper experience you met, there was way to avoid by select device before launch reaper could load different settings given by command line arguments. it's same I mention "--user-data-dir" option of chrome. users could make shortcuts for different audio devices.

in my view, audioDevice as index is a problem, even I am also the common user side (I also have the benefit by the index fallback approach)

there are no reliable way to select specific device. no solutions for advanced users who use some audio devices. I see this is a rare case so low priority.

ok It being too much now, back to original issue topic. my question was the audioDevice value was looks invalid. it's ok now.

I checked out v2.13.0 and build failed, I need to reconstruct libraries?

elieserdejesus commented 8 years ago

@teamikl

I checked out v2.13.0 and build failed, I need to reconstruct libraries?

No. In Qt creator try clean and Run qmake before compile. Sometimes I have compile problems when switching branches because the make files are using the old branch data. Another possibilty are obsolete pre-compiled headers in Qt Creator cache dirs. The clean will solve all these problems.

elieserdejesus commented 8 years ago

@jonjamcam , are you experiencing this bug in the 2.0.14 version (the master branch)?

jonjamcam commented 8 years ago

@elieserdejesus . I see the full screen mode is working differently now, so to recreate the bug I have to press F11 twice. So far it looks the bug is gone. I do further testing...

elieserdejesus commented 8 years ago

@jonjamcam and @teamikl , while testing this bug using the master branch I see another bug, but related. I hope the bug is solved now, please check the PR

elieserdejesus commented 8 years ago

Closing, the fix is integrated in master!