Skip to content
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

Macro-generated frames do not remember their previous position #2485

Closed
Pmofmalasia opened this issue Mar 21, 2021 · 16 comments · Fixed by #2682
Closed

Macro-generated frames do not remember their previous position #2485

Pmofmalasia opened this issue Mar 21, 2021 · 16 comments · Fixed by #2682
Assignees
Labels
bug tested This issue has been QA tested by someone other than the developer.

Comments

@Pmofmalasia
Copy link

Describe the bug
Before 1.8.3, the frame created by the frame5() roll option would remember where it was last docked. On closing and reopening MapTool, creating the same frame (which I believe was determined by the frame title, unsure) would then create the frame where it was last placed. Since 1.8.3, MapTool no longer remembers the frame's last position and creates it floating, as if it had been created for the first time. Note that this only occurs with restarting MapTool, closing the frame itself and then recreating it without closing MapTool correctly places the frame.

To Reproduce
Steps to reproduce the behavior:

  1. Create a frame from a macro using the frame5() roll option
  2. Dock it in a position you like
  3. Save the campaign and close MapTool
  4. Reopen MapTool and rerun the macro
  5. The frame will not be in the same place as it was docked when the campaign was saved.

Expected behavior
The frame should be created in the same place as it was docked when the campaign was saved.

MapTool Info

  • Version: 1.8.4
  • Install: New

Desktop (please complete the following information):

  • OS: Windows 10
@Irarara
Copy link
Contributor

Irarara commented Apr 14, 2021

I wanted to take a stab at this, but unfortunately had no luck.

I couldn't find any change that seemed like it might cause this change to happen (nor have I had any luck finding a way to restore the old behavior), but it seems like what is going on is that when layout data is loaded it will only load data for frames that have already been added to the DockingManager, but in 1.7.0 (and earlier) it would load everything.

In MapToolFrame, all the normal dockable frames are added before the the layout file is loaded, which is why restoring their layout works as always. Technically, loading the layout again after adding an HTMLFrame will restore its position (if it is present in layout.dat), buuut it'll also reset any other frames that have been moved since Maptool was launched.

@Azhrei
Copy link
Member

Azhrei commented Apr 14, 2021

The JIDE libraries were updated and this is likely an internal change.

If there is a close event for the JidePanel, perhaps we can catch it and then save the position so it can be reassigned when the same name is used in the future?

@Irarara
Copy link
Contributor

Irarara commented Apr 28, 2021

This came up on discord so I got curious again. I was skimming some of the old JIDE patch notes and saw this:

(Dock) Fixed the frame is not restored at the right position if they are not added when the loadLayoutData is called although the saved layout has the position for it.

This is from an old patch, but it at least seems to suggest that the old behavior was the intended behavior if the issue got fixed once before.

@Phergus
Copy link
Contributor

Phergus commented Apr 28, 2021

It would make sense that if you have read in the layout info that it would be retained in case there is info for frames not yet initialized. So did they break it in a later version or are we not retaining the info after startup?

@Irarara
Copy link
Contributor

Irarara commented Apr 28, 2021

I'd guess something broke, unless it needs to be handled differently now. As far as I can tell, nothing special was being done to deal with the layout info when it was working, besides loading and saving it.

@Phergus
Copy link
Contributor

Phergus commented Apr 28, 2021

I wonder if this might be similar to #2558. Though there are no errors so probably not.

@kwvanderlinde
Copy link
Collaborator

I changed the layout file to XML and verified that the frame's dock context is being saved correctly. I also stepped through the loading process and verified that the context is loaded again. It's even stored into the docking manager, only to be subsequently removed again somewhere up the stack (unfortunately somewhere I couldn't step through). It seems that it loads all the contexts and then removes any that don't have a corresponding frame added yet.

I also took a look through the changelog and I suspect that 3.5.10 is where the behaviour changed. Can't say for sure though.

(Dock) Fixed the layout loading issue while a context does not really exist in a new DefaultDockingManager. See the bug report here.

It looks like the best course of action would be to report a bug to JIDE about this.

@Phergus
Copy link
Contributor

Phergus commented May 21, 2021

@kwvanderlinde thanks for digging into it.

@Azhrei
Copy link
Member

Azhrei commented May 25, 2021

Perhaps an alternative is to add support for Perspectives? I don't remember those being available ten years ago 🙄 but with additional menu entries we could allow users to save/restore their layouts. (Not sure if MT-defined panels should be part of that; some discussion is required, I think.)

Another option would be to switch from using a file to using Java Preferences. On the one hand, I dislike how java.util.prefs is implemented on some platforms, but I'm wondering if the JIDE libraries use of them would be in a "lazy mode", such that they aren't accessed until and unless they are referenced. If so, they might be accessed only when a panel is opened and thus still work.

@kwvanderlinde Do you have any interest in playing around with that?

@Irarara
Copy link
Contributor

Irarara commented May 25, 2021

Chiming in because I looked at this before too, I can confirm that changing how the preferences are stored doesn't seem to change the behavior, unfortunately.

I've had a dumb/hacky idea for this in my head for awhile, but I haven't brought it up because, well, it feels dumb and hacky, but I'll throw it out there anyway (in the case an actually good solution isn't found).

Basically, you'd need to get a list of the past open macro frames, this would probably most easily be done by just writing out the names of the non-MTFrame frames to a file when Maptool exits (technically, you could parse the names right out of the layout file, but that seems like... quite a bit more work). With the names you could create and add empty frames for each before the layout is loaded, which would allow the layout data to be retained. After that, they could all be hidden, and then when adding new frames you'd just check to see if the temp frame exists and remove it before adding it properly.

I decided to try and test it with some specific macro frames and it does work. The frames' layout were restored the next time Maptool launched (whether or not the frame was ever opened the last time the client was closed). So, it seems like a viable solution, if a little weird.

@Azhrei
Copy link
Member

Azhrei commented May 25, 2021

Chiming in because I looked at this before too, I can confirm that changing how the preferences are stored doesn't seem to change the behavior, unfortunately.

You mean trying out java.util.prefs? Darn.

I've had a dumb/hacky idea for this in my head for awhile, but I haven't brought it up because, well, it feels dumb and hacky, but I'll throw it out there anyway (in the case an actually good solution isn't found).

I've thought about this as well, but if the frames where created then MT would know about them and they'd show up in getClient() calls and such. Are there frameworks that try to optimize the content of the frames and not redisplay them if they already exist? (That will currently work fine for frames that are mostly static content.)

It's also possible to add a menu option to "Reload frame layout". The docs seem to imply that doing that at any time would restore the layout, but then if the user had made changes to any of them, they'd lose those changes. It would be nice to restore just a subset of frames; ie, just the user-created ones. Hence, Perspectives.

For reference, this is the PDF of the doc.

@Irarara
Copy link
Contributor

Irarara commented May 25, 2021

You mean trying out java.util.prefs? Darn.

Yeah, I tried using loadLayoutData() and saveLayoutData(). If I understand correctly those use java.util.prefs, right?

I've thought about this as well, but if the frames where created then MT would know about them and they'd show up in getClient() calls and such. Are there frameworks that try to optimize the content of the frames and not redisplay them if they already exist? (That will currently work fine for frames that are mostly static content.)

That's a great point I hadn't thought of! Did you mean getInfo("client")? I just tested and the frames seem to not show up there until they're actually opened up with a macro.

I haven't looked too much at perspectives but I get what you're saying, might be a path there if the layout for the macro frames could be saved separately.

@Irarara
Copy link
Contributor

Irarara commented May 25, 2021

If there are no major objections or glaring issues with the solution I brought up, I would be happy to work on this. Now that I've actually tested the concept a bit, I think that approach should work well enough, even if it's a bit of a weird workaround.

@Azhrei
Copy link
Member

Azhrei commented May 25, 2021

You mean trying out java.util.prefs? Darn.

Yeah, I tried using loadLayoutData() and saveLayoutData(). If I understand correctly those use java.util.prefs, right?

No, it looks like MapToolFrame.java:565 turns off the use of Preferences, so unless you changed that, too...?

I've thought about this as well, but if the frames where created then MT would know about them and they'd show up in getClient() calls and such. Are there frameworks that try to optimize the content of the frames and not redisplay them if they already exist? (That will currently work fine for frames that are mostly static content.)

That's a great point I hadn't thought of! Did you mean getInfo("client")? I just tested and the frames seem to not show up there until they're actually opened up with a macro.

Oh, yeah — getInfo("client"). Good catch. It's great that they don't show up there, though! That means creating fake/empty frames shouldn't mess up any existing frameworks.

I haven't looked too much at perspectives but I get what you're saying, might be a path there if the layout for the macro frames could be saved separately.

Yes, the docs seem to say that individual frames can be added to a perspective, then the settings for just those frames can be saved/loaded. We could conceivably add the layout information as an entry in the campaign file so it'd always be there. Or maybe save the settings in a file using the name of the campaign so they could be automatically reloaded when the campaign was loaded. (The former has the benefit of being able to easily push the information out to clients when they connect so they get the same default layout. Only problem is that power users are not going to want their layout changed on them!)

@Irarara
Copy link
Contributor

Irarara commented May 25, 2021

No, it looks like MapToolFrame.java:565 turns off the use of Preferences, so unless you changed that, too...?

Ah, whoops, yeah, I did change that too.

@Phergus
Copy link
Contributor

Phergus commented May 27, 2021

Tested.

Also of note, if the frame is floating its position is not restored, but this is also the case for macro frames in 1.7.

Confirmed. However the size is retained between sessions.

Docked auto-hide frames do not retain their position and this was the same in 1.7.

No new problems seen.

@Phergus Phergus added the tested This issue has been QA tested by someone other than the developer. label May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug tested This issue has been QA tested by someone other than the developer.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants