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

Stop function final reviwe #109

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pradeepkumara
Copy link

Description

I have changed the stop funciton according to the specification given in the task
Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as
    expected)
  • Documentation (update or new)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can
reproduce. Please also list any relevant details for your test configuration

Testing Checklist

  • Tested in latest Chrome
  • Tested in latest Safari
  • Tested in latest Firefox

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have requested a review from ... on the Pull Request

Copy link

🐋 PR Preview!
Preview at https://thoth-tech.github.io/SplashkitOnline/pr-previews/109
for commit 7a55d22

Copy link
Contributor

@WhyPenguins WhyPenguins left a comment

Choose a reason for hiding this comment

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

Hey @pradeepkumara! This is a decent attempt, but it needs a lot of fixes before it can be considered complete; right now it doesn't actually work. Have another check over the code and see if you can patch it up based on the suggestions 😃

Comment on lines +103 to 111
<button type="button" id="stopProgram">
<i class="bi bi-stop-fill"></i>
<span>Stop</span>
</button>
<button type="button" id="collapsedStopProgram" title="Stop Program">
<i class="bi bi-stop-fill">Terminate</i>
</button>

<button type="button" id="stopProgram" style="display:none"><i class="bi bi-pause-fill">Pause</i></button>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is working as intended - we end up with two stop buttons by accident, and the pause button is hidden since it has the same ID stopProgram:
image
The collapsedStopProgram button should only be visible when the interface is collapsed, and the normal pause button should still be visible.
Maybe something like this?
image

Copy link
Author

Choose a reason for hiding this comment

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

thank you for the feedback, I will change this.

Comment on lines +388 to +405
* Checks if a program is currently running
* @returns {boolean} - True if a program is running, false otherwise
*/
function isProgramRunning() {
// Replace this with the actual logic to check program state
return runtimeManager.isRunning();
}

/**
* Displays a notification to the user
* @param {string} message - The message to display
* @param {string} type - The type of the notification ("info", "warning", "error")
*/
function showNotification(message, type) {
// Replace this with your actual notification logic
console.log(`[${type.toUpperCase()}] ${message}`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These don't make sense, I'm assuming AI generated?

Comment on lines +354 to +365
* Terminates the currently running program
*/
function terminateProgram() {
try {
// Logic to terminate the running process
runtimeManager.terminate();
updateRuntimeButtons(false); // Disable runtime buttons after stopping
} catch (error) {
throw new Error("Program termination failed: " + error.message);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, AI code that shouldn't be in here.

Comment on lines +317 to +374
terminateProgram();
});

document.getElementById("collapsedStopProgram").addEventListener("click", () => {
terminateProgram();
});

// Attach event listeners for Stop buttons
initializeEventListeners();

function initializeEventListeners() {
const stopButton = document.getElementById("stopProgram");
const collapsedStopButton = document.getElementById("collapsedStopProgram");

stopButton.addEventListener("click", handleStopProgram);
collapsedStopButton.addEventListener("click", handleStopProgram);
}

/**
* Handles the stop button click event
*/
function handleStopProgram() {
if (!isProgramRunning()) {
showNotification("No program is currently running.", "warning");
return;
}

try {
terminateProgram();
showNotification("Program stopped successfully.", "info");
} catch (error) {
console.error("Error while stopping the program:", error);
showNotification("Failed to stop the program. Please try again.", "error");
}
}

/**
* Terminates the currently running program
*/
function terminateProgram() {
try {
// Logic to terminate the running process
runtimeManager.terminate();
updateRuntimeButtons(false); // Disable runtime buttons after stopping
} catch (error) {
throw new Error("Program termination failed: " + error.message);
}
}

/**
* Updates the runtime control buttons based on the program state
* @param {boolean} isRunning - Whether the program is currently running
*/
function updateRuntimeButtons(isRunning) {
toggleButtonState("stopProgram", isRunning);
toggleButtonState("collapsedStopProgram", isRunning);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should all be in editorMain.js - there's a lot of framework code already there to help. Have a look in the functions setupIDEButtonEvents and updateButtons 😃

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