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

Allow opening multiple units in tabs #1673

Merged
merged 15 commits into from
Jan 2, 2025
Merged

Conversation

pavelbraginskiy
Copy link
Member

@pavelbraginskiy pavelbraginskiy commented Dec 31, 2024

Many people have requested the ability to have many units open at once.

image

Everything seems to just work the way you might expect it to, but I'd like to get this to the testers to try to break it as soon as possible.

This shouldn't impact MekHQ, and when I opened MHQ MML it didn't instantly explode, but I don't know how to use MHQ so testing is necessary to make sure everything really does still work there.

You can shift-click the close-tab button to skip the "Would you like to save" dialog.

When you close MML, the state of the tabs is saved to a hidden directory in the user folder.

If the startup mode is set to "Restore Tabs", a user can pick up right where they left off, even including any unsaved work.

Future work:

  • Allow opening all the units from a MUL file in tabs.
  • Include a setting to periodically save your tab state, so you can recover your work even if your computer crashes.

Copy link

codecov bot commented Dec 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 2.14%. Comparing base (52fd6fc) to head (a2abcdd).
Report is 27 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##             master   #1673      +/-   ##
===========================================
- Coverage      2.16%   2.14%   -0.02%     
  Complexity      209     209              
===========================================
  Files           268     270       +2     
  Lines         30930   31247     +317     
  Branches       5289    5328      +39     
===========================================
+ Hits            669     670       +1     
- Misses        30104   30420     +316     
  Partials        157     157              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pavelbraginskiy pavelbraginskiy marked this pull request as draft January 1, 2025 12:28
@pavelbraginskiy pavelbraginskiy marked this pull request as ready for review January 1, 2025 12:42
@Sleet01
Copy link
Collaborator

Sleet01 commented Jan 1, 2025

@pavelbraginskiy Can you grab the rs_font entry from your megameklab.properties file? I don't seem to have any fonts that support the "➕" and "❌" symbols.

Edit: NM, that's just the record sheet font. Where is MML setting its default display font...

CConfig.getMainUiWindowSize(this).ifPresent(this::setSize);
CConfig.getMainUiWindowPosition(this).ifPresent(this::setLocation);

// ...and save that size nad position on exit
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"and"

return true;
}

private void restrictToScrenSize() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Screen"

Comment on lines 263 to 293
//<editor-fold desc="MenuBarOwner interface implementation">
@Override
public JFrame getFrame() {
return this;
}

@Override
public Entity getEntity() {
return currentEditor().getEntity();
}

@Override
public String getFileName() {
return currentEditor().getFileName();
}

@Override
public boolean hasEntityNameChanged() {
return currentEditor().hasEntityNameChanged();
}

@Override
public void refreshMenuBar() {
menuBar.refreshMenuBar();
}

@Override
public MenuBar getMMLMenuBar() {
return menuBar;
}
//</editor-fold>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think IDE-specific formatting comments should be included in the main repo.

) {
ps.println(((Mek) editor.getEntity()).getMtf());
} catch (Exception e) {
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely needs logging of the exception; no silent file errors.

try {
BLKFile.encode(unitFile.getPath(), editor.getEntity());
} catch (EntitySavingException e) {
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

editor.reloadTabs();
editor.refreshAll();
editors.add(editor);
} catch (EntityLoadingException ignored) {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely want logging of exceptions while attempting to read unit files.


try {
Entity loadedUnit = new MekFileParser(newFile).getEntity();
newFile.delete();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that we shouldn't delete any files until we've completed all actions in the try block.

Copy link
Collaborator

@Sleet01 Sleet01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the stuff I commented on, I think we need to add the standard tab-related menu items and symantics:

  • File -> Close, to mirror the X button function;
  • File -> Open, similar to load but replaces the current tab's contents with the selected unit.
  • Wrap the selected tab around instead of creating infinite new tabs, when using CTRL+Tab and right arrow in the tab bar.

Otherwise, this looks fantastic and I'm hyped to start using the tabs!

Copy link
Collaborator

@Sleet01 Sleet01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks really good!

@Sleet01 Sleet01 merged commit 407c871 into MegaMek:master Jan 2, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants