Skip to content

Conversation

@vpstackhub
Copy link
Contributor

@jamesmarkchan

Summary

Implements persistence and runtime propagation for RenderFrequencyMode (Issue #78).

Details

  • Added @Enumerated(EnumType.STRING) to BenchmarkOperation.renderMode
  • Added safe getter defaulting to PER_SAMPLE for backward compatibility
  • Ensured BenchmarkWorker sets renderMode for read/write operations
  • Verified correct GUI behavior and render update frequencies (100ms–1000ms)
  • Confirmed EnumType mapping eliminates previous conversion errors

Verification

  • Clean build/run via ant
  • Verified graphs reflect selected frequency per mode
  • No regressions or runtime exceptions observed

Ready for review and merge into dev.

@vpstackhub vpstackhub changed the base branch from main to dev October 20, 2025 23:54
@jamesmarkchan
Copy link
Member

jamesmarkchan commented Nov 9, 2025

@vpstackhub a lot of the code looks like it is in good shape!

Here are items to follow up on:

  1. relocate render mode persistence to parent benchmark record instead of the benchmark operation - the render mode is likely to be the same for the entire benchmark so we should not need to store it for each child operation and can just persist it once.

  2. add close button to the bottom right of the dialog, it can just hide the window, would be good to resize the window to just frame the option and close button. good practice to not just rely on the upper right X to close the dialog.
    image
    image
    drag this over to the design tab...
    image

  3. add logic to save render mode to our properties file and load set it when the app is started so basically if a user selects a render mode and closes and restarts the app the mode will be persisted through our properties file. see App.saveConfig and App.loadConfig. also when adding the call to the user selecting the drop down note the hasFocus() check prevents to setting on load
    image

  4. the per timeframe options do not appear to be working as expected, note unlike the per operation the graphing of the per time frame will need to be managed as a list that is populated when samples are not graphed and then when the graphing interval occurs they are graphed and the list cleared / reset so it can be repopulated during the next interval.

  5. when we currently do a read and write benchmark when it is complete we see both graph sets graphed from the read and write operation. when we are in per operation mode we see first the read benchmark plotted and then the write benchmark plotted. ideally it might be good to have the same behavior regardless of the mode we use to plot.

  6. looks like there are conflicts with the dev branch, good to pull from the dev branch and merge in the changes from there. if there are any questions about the changes coming from the dev branch have me join you for pulling dev into this branch. it is normal and recommended practice to resolve conflicts on a branch by pulling from a parent branch before merging back into the parent branch.

action for me:

  • I used the wrong mouseReleased event handler when creating the AdvanceOptionsFrame.java, I'll correct and push that to the branch for us now.

@jamesmarkchan
Copy link
Member

@vpstackhub in case this is your first time, to pull changes from dev into the current branch and resolve merge conflicts from this branch you'd run the command git merge dev and then resolve the conflicts locally and commit the merged changes.

@vpstackhub
Copy link
Contributor Author

Thank you James! quick update: I added the dedicated Close button to the Advanced Options dialog and verified it hides the window properly. I also saw your note about merging dev .I actually synced earlier today, so the branch should already be up to date. Would you like me to

  1. proceed next with moving RenderFrequencyMode persistence from BenchmarkOperation to the parent Benchmark and
  2. implementing save/load logic in the properties file so the selected mode is restored on startup? Or would you prefer I prioritize something else first?

@jamesmarkchan
Copy link
Member

jamesmarkchan commented Nov 15, 2025

Thank you James! quick update: I added the dedicated Close button to the Advanced Options dialog and verified it hides the window properly. I also saw your note about merging dev .I actually synced earlier today, so the branch should already be up to date. Would you like me to

  1. proceed next with moving RenderFrequencyMode persistence from BenchmarkOperation to the parent Benchmark and
  2. implementing save/load logic in the properties file so the selected mode is restored on startup? Or would you prefer I prioritize something else first?

Good Day @vpstackhub yes those two above items sounds like a good thing to do as part of this item. I believe when the merge is successfully resolved this pull request view will no longer show this message...

image

possible that after merging locally you still need to push the resolution from your local workstation to the github master repo. remember always okay to give the other developer a call to have them merge code with you to avoid any lost work.

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.

5 participants