Mudlet icon indicating copy to clipboard operation
Mudlet copied to clipboard

Add: sysProfileFocusChangeEvent

Open SlySven opened this issue 3 years ago • 3 comments

This is an attempt to close #5960 by detecting when the currently active host changes when multi-playing and issuing an event to replace the ones lost when I revised another part of the codebase to NOT issue a NAWS report or a sysWindowResizeEvent when the size of the main window was NOT actually changed but just shown/hidden.

Signed-off-by: Stephen Lyons [email protected]

SlySven avatar Aug 29 '22 13:08 SlySven

Hey there! Thanks for helping Mudlet improve. :star2:

Test versions

You can directly test the changes here:

  • linux: https://make.mudlet.org/snapshots/74d2fb/Mudlet-4.16.0-testing-pr6267-4f3df2c1-linux-x64.AppImage.tar
  • osx: https://make.mudlet.org/snapshots/de8972/Mudlet-4.16.0-testing-pr6267-4f3df2c1.dmg
  • windows: https://make.mudlet.org/snapshots/7b1faa/Mudlet-4.16.0-testing-pr6267-19df819f-windows.zip

No need to install anything - just unzip and run. Let us know if it works well, and if it doesn't, please give details.

This doesn't work yet - my attempt at getting the Geyser system to repaint the windows does not work to cure the problem that the sample code by the OP in the associated issue demonstrates. It needs someone with more Lua/Geyser-fu to work on it - so I think I need to call :mega: in @demonnic ...!

SlySven avatar Aug 29 '22 13:08 SlySven

Related discussion in #6273 suggest maybe a combined first step would be to refactor/bring to a united position all known ways to change the current profile. Afterwards, title can be updated accordingly (my issue) and the event may be issued (your issue).

Kebap avatar Sep 07 '22 21:09 Kebap

Looking into how Mudlet is currently operating I realise that @Kebap has a very valid point - and I need to hook up all the uses of:

  • (void) mudlet::activateProfile(Host*)
  • (void) mudlet::mpTabBar::setCurrentIndex(int)

as well as the existing (void) mudlet::slot_tabChanged(int) code I put together. to generate the new sysProfileChangeEvent, ~~converting this PR back to a draft until I have~~ it is now got this sorted out...

SlySven avatar Nov 27 '22 01:11 SlySven

Warnings
:warning: PR makes changes to 15 source files. Double check the scope hasn't gotten out of hand

Generated by :no_entry_sign: dangerJS against 19df819f65e26ec95385992747ab08077208706a

github-actions[bot] avatar Dec 01 '22 06:12 github-actions[bot]

How will this work?

Would any code that reacted to sysWindowResizeEvent now needs to react to sysProfileFocusChangeEvent as well or things won't look right?

vadi2 avatar Dec 01 '22 07:12 vadi2

This is an attempt to close https://github.com/Mudlet/Mudlet/issues/5960

That issue was already solved. As it turned out, the window size was calculated wrong.

Which issue does this PR fix now?

Kebap avatar Dec 01 '22 10:12 Kebap

#6273 seems improved / solved with this PR. #6449 seems unaffected / not solved with this.

Please let me know what else is there to test.

Kebap avatar Dec 01 '22 11:12 Kebap

How will this work?

Would any code that reacted to sysWindowResizeEvent now needs to react to sysProfileFocusChangeEvent as well or things won't look right?

No in the sense that some code could have been relying on the sysWindowResizeEvent to know when the profile was shown/hidden when multi-playing but multi-view mode was not switched on rather then using that signal to redraw because the screen dimensions were changed. This was what I commented on in #5887 when I "fixed" the former event so that it and the NAWS signalling were not being done when a profile was shown/hidden but only when there was a resize.

This PR comes about because I foresaw that there could/would be a need to know when a profile gained/lost focus when multiplying - however before I got to the point it is at now Leris/Kebap pointed out that the Mudlet application as a whole was not properly keeping track of which profile was "in focus" (because the title bar was not being updated to reflect the active one) and that solving one should fix both things.

#6449 seems unaffected / not solved with this.

After this PR clicking on the TConsole/(pair of TTextEdits in it) of a UserWindow (i.e. the floating/dockable windows) the focus will stay in the TCommandLine of that UserWindow if it has been enabled; it will also stay there as well when clicking within that same TCommandLine. However I have found that the same is NOT true of MiniConsoles (those that are embedded within the TMainConsole) as I realised I have missed one chunk of code, that being a mouseReleaseEvent(QMouseEvent*) for the TCommandLine class.

Furthermore, the focus largely stays on a secondary (additional) command line added to the main console, though that is also a bit off because of the aforesaid missing release event handler. OTOH Clicking on the floating/dockable mapper and both the buttons/menus AND any empty space in a floating/dockable toolbar returns the focus to the main command line and sets the active profile to the correct one - before this PR clicking on empty space in such a toolbar didn't have any effect.

SlySven avatar Dec 01 '22 15:12 SlySven

some code could have been relying on the sysWindowResizeEvent to know when the profile was shown/hidden

Is there such code though? The original code linked in the issue wasn't doing of that, it was just broken due to wrong data being reported to it 🤔

vadi2 avatar Dec 01 '22 16:12 vadi2

For #6450, clicking inside of map while on different profile, this does seem to fully switch over to the other profile properly.

atari2600tim avatar Dec 01 '22 17:12 atari2600tim

Waiting on: #6451 ...

SlySven avatar Dec 08 '22 20:12 SlySven

Oops, it seems we don't have a "This PR works but is waiting for another PR to fix the development codebase so that the CI builds work"...!

SlySven avatar Dec 08 '22 20:12 SlySven

I'm still a bit unclear on the need for sysProfileFocusChangeEvent. What code is expected to make use of it and at what times, and why doesn't the existing sysWindowResizeEvent one cover it?

No in the sense that some code could have been relying on the sysWindowResizeEvent to know when the profile was shown/hidden when multi-playing but multi-view mode was not switched on rather then using that signal to redraw because the screen dimensions were changed.

Where can I find this code?

There are also semi-related fixes to focus tracking of userwindows and command lines - that would be better served and easier to merge if it was a separate PR, I reckon.

vadi2 avatar Dec 12 '22 10:12 vadi2

I'm still a bit unclear on the need for sysProfileFocusChangeEvent. What code is expected to make use of it and at what times, and why doesn't the existing sysWindowResizeEvent one cover it?

It is foreseeable - at least I can see it - that when multi-playing a profile might need to know whether it has the player's attention/focus - especially if they are NOT using multi-view mode {imagine that the profile is not the one being shown and a monster or enemy attacks the character in that profile}. As such this new event provides something (a signal that the profile has gained OR lost focus) that a script/package can use for that. Previously they might have been able to use the sysWindowResizeEvent - but since #5887 that no longer fires when the user clicks between tabs/profiles so cannot be used to achieve it.

Originally I thought that the elimination of the bogus NAWS messages was the cause of #5960 - but it turns out it wasn't. At the same time though various parties, @Kebap included, noted that clicking on various UI elements of a profile was not causing the focus to be switched back to that profile if more than one was active; as such this PR morphed into something that could fix that as well - because the areas of the codebase involved are interrelated. As such this PR should clean things up because the process of switching between profiles has been streamlined IMHO.

SlySven avatar Dec 12 '22 11:12 SlySven

Thanks, the need for the event makes sense! I see it's making use of 0's and 1's - could you describe how that works so @Mudlet/lua-interface can review it appropriately?

vadi2 avatar Dec 12 '22 14:12 vadi2

Thanks, the need for the event makes sense! I see it's making use of 0's and 1's - could you describe how that works so @Mudlet/lua-interface can review it appropriately?

No need, the 0/1 are how we pass false/true boolean values (for "Lost"/"Gained" focus respectively) in the Mudlet TEvent code - by the time the users/@Mudlet/lua-interface get to see them they will be the latter Lua values...

SlySven avatar Dec 12 '22 14:12 SlySven

:books: Provisional documentation in Area 51: https://wiki.mudlet.org/w/Area_51#sysProfileFocusChangeEvent.2C_PR6267_open.29 .

SlySven avatar Dec 16 '22 15:12 SlySven

Otherwise looks solid to me.

vadi2 avatar Dec 17 '22 09:12 vadi2