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

New extension: OfflineTimeTracker #1093

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 17, 2023

Description

Calculation of the time passed (in and out of the game).
By LazyPanda(Mahan-ashrafi)

How to use the extension

for calculating the time between 2 points in time.
no matter the game/app is open or closed , if you have a timestamp saved when the game/app is open it will keep track of the time passed

Checklist

  • I've followed all of the best practices.
  • I confirm that this extension can be integrated to this GitHub repository, distributed and MIT licensed.
  • I am aware that the extension may be updated by anyone, and do not need my explicit consent to do so.

What tier of review do you aim for your extension?

Reviewed

Example file

ExampleProject.zip

Extension file

OfflineTimeTrackerExtension.zip

@github-actions github-actions bot requested a review from a team as a code owner November 17, 2023 19:04
@github-actions github-actions bot added the ✨ New extension A new extension label Nov 17, 2023
@github-actions github-actions bot mentioned this pull request Nov 17, 2023
3 tasks
@VegeTato VegeTato added the 👨‍👩‍👧‍👦 Community extension An extension submission to be merged ASAP with a lightweight review. label Dec 1, 2023
@VegeTato
Copy link
Contributor

Hello @Mahan-Ashrafi 👋 thank you for submitting this amazing extension ! ❤️
I noticed few things in the extension that need to be fixed:
1- Please follow the best extension practices, for example:
Your storage name "timestamp" must be __TimeStamp.timestamp same thing apply to timers names.
Your variable name Loadedname should be __TimeStamp.Loadedname same thing apply to all variables.
2- You are using log message action in the extension, this is not required for the extension to work, so it's better to delete them.
3- If possible, please add comments explaining what the event do, so if any user wants to take a look at the extension events, can understand what's going around.
4- Use PascalCase in your parameters names, for example:
parameter name must be Name, same thing apply for the parameters first and second.

Fix this issues and let me know, so I can review the extension 👍

@Mahan-Ashrafi
Copy link

Hello @Mahan-Ashrafi 👋 thank you for submitting this amazing extension ! ❤️ I noticed few things in the extension that need to be fixed: 1- Please follow the best extension practices, for example: Your storage name "timestamp" must be __TimeStamp.timestamp same thing apply to timers names. Your variable name Loadedname should be __TimeStamp.Loadedname same thing apply to all variables. 2- You are using log message action in the extension, this is not required for the extension to work, so it's better to delete them. 3- If possible, please add comments explaining what the event do, so if any user wants to take a look at the extension events, can understand what's going around. 4- Use PascalCase in your parameters names, for example: parameter name must be Name, same thing apply for the parameters first and second.

Fix this issues and let me know, so I can review the extension 👍

Thx ill fix it in a minute

@github-actions github-actions bot force-pushed the extension/Mahan-Ashrafi/1092 branch from 9d354d7 to d642733 Compare March 22, 2024 14:06
@Mahan-Ashrafi
Copy link

@VegeTato Here You go !
updated the description , icon ( to better suit the title )
if it needs anything else alert me

@D8H
Copy link
Contributor

D8H commented Mar 22, 2024

Your storage name "timestamp" must be __TimeStamp.timestamp same thing apply to timers names.

Actually, it would just be __TimeStamp. It wouldn't make much sense to create several storages for an extension.

3- If possible, please add comments explaining what the event do, so if any user wants to take a look at the extension events, can understand what's going around.

Where do you think a comment is needed? This extension only use storage actions and a subtraction, this looks straightforward to me.

@Mahan-Ashrafi
Copy link

Your storage name "timestamp" must be __TimeStamp.timestamp same thing apply to timers names.

Actually, it would just be __TimeStamp. It wouldn't make much sense to create several storages for an extension.

3- If possible, please add comments explaining what the event do, so if any user wants to take a look at the extension events, can understand what's going around.

Where do you think a comment is needed? This extension only use storage actions and a subtraction, this looks straightforward to me.

I added comments to the extension code
Is that needed ?
And i already uploaded the update

"textG": 0,
"textR": 0
},
"comment": "Creation of a timestamp with the given name"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is paraphrasing the action description. It won't help readers.
You can remove all of them, sorry for the burden.

@D8H
Copy link
Contributor

D8H commented Mar 22, 2024

Your storage name "timestamp" must be __TimeStamp.timestamp same thing apply to timers names.

Actually, it would just be __TimeStamp. It wouldn't make much sense to create several storages for an extension.

Your extension name is TimeStampManager so the storage name should even be __TimeStampManagerExtension. "Timestamp" is only one word, so the "S" should be lower case.
I think the extension name is a bit vague and technical. Users may not understand it. What do you think about "OfflineTimeChecker" or something like this? The idea is to find a name that explains the main feature rather than how it's implemented.

@Mahan-Ashrafi
Copy link

Maybe OfflineTimeTracker
I like it to be timestamp for people to learn some technical terms
This extension will be used by advanced users mostly

@D8H
Copy link
Contributor

D8H commented Mar 22, 2024

Maybe OfflineTimeTracker I like it to be timestamp for people to learn some technical terms This extension will be used by advanced users mostly

Indeed, "OfflineTimeTracker" is great. Yes, you can still use "timestamp" in the actions.

@Mahan-Ashrafi
Copy link

Maybe OfflineTimeTracker I like it to be timestamp for people to learn some technical terms This extension will be used by advanced users mostly

Indeed, "OfflineTimeTracker" is great. Yes, you can still use "timestamp" in the actions.

Ohh i can keep timestamp 😅
Ok ill be back with the update

@github-actions github-actions bot force-pushed the extension/Mahan-Ashrafi/1092 branch from d642733 to 43e9270 Compare March 23, 2024 09:49
@github-actions github-actions bot changed the title New extension: Timestamp manager New extension: OfflineTimeTracker Mar 23, 2024
@Mahan-Ashrafi
Copy link

@VegeTato @D8H Done !

@D8H
Copy link
Contributor

D8H commented Mar 27, 2024

Please attach the new version of the example. It's the example that is used to do the review.

@Mahan-Ashrafi
Copy link

Please attach the new version of the example. It's the example that is used to do the review.

I just checked I did
You mean create a new example ?

@D8H
Copy link
Contributor

D8H commented Mar 28, 2024

Please attach the new version of the example. It's the example that is used to do the review.

I just checked I did You mean create a new example ?

I can't find it. I mean the same example but which uses the new extension version.

@Mahan-Ashrafi
Copy link

Please attach the new version of the example. It's the example that is used to do the review.

I just checked I did You mean create a new example ?

I can't find it. I mean the same example but which uses the new extension version.

https://github.com/GDevelopApp/GDevelop-extensions/files/14731017/ExampleProject.zip

Its in this page already @D8H

@D8H
Copy link
Contributor

D8H commented Mar 28, 2024

https://github.com/GDevelopApp/GDevelop-extensions/files/14731017/ExampleProject.zip

Its in this page already @D8H

This is the link to the example that uses the 1st version of the extension, right?

Edit: sorry, it seems the bot updated the links.

@D8H
Copy link
Contributor

D8H commented Mar 30, 2024

  • Both PR

    • I think the value should not be rounded.
      • It's easy for extension users to round it if they need to, but impossible to "unround" it.
    • What do you think about automatically saving the timestamp every minute by default?
      • onScenePreEvents can be used for this.
      • An action could allow to change the frequency to 1 per second or 1 per hour.
  • This PR

    • The timestamp name is to handle several users?
      • If the save is done automatically, an action could allow to change the name to use to keep this feature
  • NEW EXTENSION - Offline Time #1211

    • @TheGemDev uses JavaScript but it doesn't seem necessary. It's better to keep using events.
    • It has an expression that allows to get the offline time in days, hours, minutes or seconds.
      • I think it make sense to use different units
      • An expression with the unit in parameter is not practical, it's better to have 4 expressions directly.
      • A condition could be useful, the unit could be a parameter in this case.

@Mahan-Ashrafi
Copy link

Mahan-Ashrafi commented Mar 30, 2024

  • Both PR

    • I think the value should not be rounded.
      • It's easy for extension users to round it if they need to, but impossible to "unround" it.
    • What do you think about automatically saving the timestamp every minute by default?
      • onScenePreEvents can be used for this.
      • An action could allow to change the frequency to 1 per second or 1 per hour.
  • This PR

    • The timestamp name is to handle several users?
      • If the save is done automatically, an action could allow to change the name to use to keep this feature
  • NEW EXTENSION - Offline Time #1211

    • @TheGemDev uses JavaScript but it doesn't seem necessary. It's better to keep using events.
    • It has an expression that allows to get the offline time in days, hours, minutes or seconds.
      • I think it make sense to use different units
    • An expression with the unit in parameter is not practical, it's better to have 4 expressions directly.
    • A condition could be useful, the unit could be a parameter in this case.

Ok ill delete the rounding part as it removes the precision.

So whats the purpose of saving a timestamp then if you want to have autosave ?
Its better to save it whenever the user wants it to.

So you mean add an action to change a timestamps name?

@D8H
Copy link
Contributor

D8H commented Mar 30, 2024

So whats the purpose of saving a timestamp then if you want to have autosave ?

My point is that users don't know when they should save them as there is no way to know when the game is closed and if they want to compare timestamps during the same sessions they don't need an extension and should probably use timers.
So, the extension should do it automatically at a given interval.

@Mahan-Ashrafi
Copy link

Mahan-Ashrafi commented Mar 30, 2024

So whats the purpose of saving a timestamp then if you want to have autosave ?

My point is that users don't know when they should save them as there is no way to know when the game is closed and if they want to compare timestamps during the same sessions they don't need an extension and should probably use timers.
So, the extension should do it automatically at a given interval.

Timestamps make it easier to manage and track time in and out of the game.
So what im going to do is keep my stuff as they are and add another expression to auto save with the given interval.
Auto saving timestamps mostly have no use case and is not the main idea of the extension.

@TheGemDev
Copy link
Contributor

Auto saving timestamps mostly have no use case and is not the main idea of the extension.

i think auto saving is essential.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👨‍👩‍👧‍👦 Community extension An extension submission to be merged ASAP with a lightweight review. ✨ New extension A new extension
Projects
Status: Needs changes
Development

Successfully merging this pull request may close these issues.

4 participants