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

feat(ui): updates + dynamically sized window #16

Merged
merged 21 commits into from
Aug 20, 2023
Merged

feat(ui): updates + dynamically sized window #16

merged 21 commits into from
Aug 20, 2023

Conversation

Moosems
Copy link
Member

@Moosems Moosems commented Aug 14, 2023

Need to still utilize the progressbar, update docs, and clean code more but this is a good start. Please: DO NOT TOUCH. @im-coder-lg, please comment that you've read this.

Closes #15

Need to still utilize the progressbar, update docs, and clean code more but this is a good start. Please: DO NOT TOUCH. @im-coder-lg, please comment that you've read this.
@Moosems
Copy link
Member Author

Moosems commented Aug 14, 2023

Again, NO TOUCHY!

@Moosems
Copy link
Member Author

Moosems commented Aug 14, 2023

I would love it if eventually we used a web-scraper rather than using an API to make it scalable and free forever.

@Moosems
Copy link
Member Author

Moosems commented Aug 14, 2023

Artifact build is not working for me but local source running is successful.

@im-coder-lg
Copy link
Member

@Moosems I saw this, and as you said, I no touchy.

I would love it if eventually we used a web-scraper rather than using an API to make it scalable and free forever.

How about migrating to PyOWM? It seems to use the API but also format the data too, so we could perhaps migrate to that.

I'll keep my eye on this, and if you need any help, just ask.

@im-coder-lg
Copy link
Member

Artifact build is not working for me but local source running is successful.

I'll test it on my computer as well as my cloud Gitpod instance.

@Moosems about the dynamic scaling, are you going to try to update the values after a search for the city is done? I did that on the dynamic-scaling branch, and it works well. I think this is the way most could go, but if you have another idea, can you delete the branch? We don't have a use for it anymore.

I'm updating the title, description and adding this to the repo project for tracking progress. Just slight changes.

@im-coder-lg im-coder-lg changed the title UI Updates feat(ui): updates + dynamically sized window Aug 15, 2023
@im-coder-lg im-coder-lg added the enhancement New feature or request label Aug 15, 2023
@not-nef
Copy link
Member

not-nef commented Aug 15, 2023

hello

@sumeshir26
Copy link
Member

hello

hi, good to see you back

@not-nef
Copy link
Member

not-nef commented Aug 15, 2023

i kinda lost motivation for programming after that one bug in notes that i couldn’t fix

@sumeshir26
Copy link
Member

i kinda lost motivation for programming after that one bug in notes that i couldn’t fix

tkinter is frustrating sometimes. ive kinda moved on from tkinter programming at this point, and am learning newer things, so even i dont contribute much these days.

@not-nef
Copy link
Member

not-nef commented Aug 15, 2023

i just think that tkinters simplicity is nice so i am kinda hanging onto it

Couldn't load before the results came in so effectively useless.
@Moosems
Copy link
Member Author

Moosems commented Aug 15, 2023

Will make settings UI.

@Moosems
Copy link
Member Author

Moosems commented Aug 15, 2023

Need to set up a simple file system, saving to it, and update the label info based on the systems.

Moosems and others added 2 commits August 15, 2023 09:08
@im-coder-lg
Copy link
Member

@Moosems I added sv_ttk to requirements file since it was missing, now you can use build artifacts directly I think.

@im-coder-lg
Copy link
Member

Wait, I know there is an error with the artifact, I know the fix. 1 min!

@im-coder-lg
Copy link
Member

I reverted my changes. @Moosems the artefacts don't recognise sv_ttk even after my changes. Can you look into it?

@Moosems
Copy link
Member Author

Moosems commented Aug 15, 2023

@im-coder-lg Wait to touch the PR until I'm done please, still making some large changes ;).

@Moosems
Copy link
Member Author

Moosems commented Aug 15, 2023

I'll get the artifacts fixed, don’t worry.

@Moosems
Copy link
Member Author

Moosems commented Aug 15, 2023

Mac builds run fine from artifacts but now Linux is acting up.

@im-coder-lg
Copy link
Member

Also, can you change the City label text's variables to the one I suggested? At least the name would be enough, so it auto-corrects the city name after receiving it.

What?

	    cityname = data["name"] + ", " + data["sys"]["country"]
        # Set the city name
        self.cityname.configure(text=f"City: {cityname}")

This. If you add this to main.py, the proper city name will be taken from the API.

@im-coder-lg Wait to touch the PR until I'm done please, still making some large changes ;).

Okay, but do it fast! Linux builds don't work due to the addition of Sun Valley.

I'll get the artifacts fixed, don’t worry.

Okay, but make sure the .spec files are proper.

Mac builds run fine from artifacts but now Linux is acting up.

Have you checked the .spec files? Maybe we must add sv_ttk as a hidden import(though it didn't work, but worth a try).

@im-coder-lg Note that using different Springfield's (there's multiple cities named Springfield in the US) returns the same data. It makes no distinction between those in different states.
@Moosems
Copy link
Member Author

Moosems commented Aug 15, 2023

You are clear to work on the Linux builds. Try to keep changes minimal if possible.

@Moosems
Copy link
Member Author

Moosems commented Aug 15, 2023

Okay, but make sure the .spec files are proper.

I only added hooks. Would you open a discussion asking for thoughts on the PyInstaller repo?

@im-coder-lg
Copy link
Member

Okay, but can't do today :( got too much of backlog to finish here. I still don't know how it accumulates. 🤔

Of course, I'll check with PyInstaller devs on this behaviour. I might have to mention this PR there, but we'll see. Add your changes and let's wait and watch.

@Moosems
Copy link
Member Author

Moosems commented Aug 16, 2023

I'd give them the hook file, spec file, and then the traceback and leave it there, no need to link a big PR.

@im-coder-lg
Copy link
Member

im-coder-lg commented Aug 19, 2023

Posted a discussion over at PyInstaller: https://github.com/orgs/pyinstaller/discussions/7869

I linked the repo and the files, and only that, so this PR isn't linked but is accessible. I hope I get an answer to the issue soon.

@Moosems
Copy link
Member Author

Moosems commented Aug 19, 2023

There was a response. @im-coder-lg could you have the Linux build use a newer Python version?

@Moosems
Copy link
Member Author

Moosems commented Aug 19, 2023

3.9 or sooner.

@Moosems
Copy link
Member Author

Moosems commented Aug 19, 2023

Or maybe a from __future__ import annotations would fix that.

@im-coder-lg
Copy link
Member

We would have to rebase from main. I'll manage that mess, if you want me to. The workflow can be updated only on the main branch, so we gotta rebase, no questions asked.

@im-coder-lg
Copy link
Member

Oh, fixed it with GitHub's UI. I thought we had to use Git :/

@im-coder-lg
Copy link
Member

b2311101-24dd-4c14-9681-547f9782adc1.online-video-cutter.com.mp4

IT WORKS!!!!!

yeah!

Finally, I pulled the recording off. Not so fancy as before, Clipchamp straight up reloaded to the login site over and over again.

@Moosems
Copy link
Member Author

Moosems commented Aug 20, 2023

Ready to merge?

Copy link
Member

@im-coder-lg im-coder-lg left a comment

Choose a reason for hiding this comment

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

Schway!

Approved! 👍

self.cityname.grid(row=1, column=0, columnspan=2)

self.searchbar = Entry(self.main_frame, width=42)
self.searchbar.grid(row=2, column=0, columnspan=2, padx=10, pady=10)
self.bind("<Return>", self.OWMCITY)
Copy link
Member

Choose a reason for hiding this comment

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

It is good to notice this! I tried using the same bind() function, but it didn't work. Maybe because I had to use command= self.OWMCITY. I'll test this after lunch.

main.py Outdated
Comment on lines 277 to 279

# Set the city name
self.cityname.configure(text=f"City: {city}")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Set the city name
self.cityname.configure(text=f"City: {city}")
cityname = data["name"] + ", " + data["sys"]["country"]
# Set the city name
self.cityname.configure(text=f"City: {cityname}")

SUGGESTION

This change is simple: it takes the name of the city from the API(data["name") and adds it with the country code(data["sys"]["country"]). Now, even if a person enters a redundant name, it will change to the official name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you please put it all in the f-string?

Copy link
Member

Choose a reason for hiding this comment

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

Well, isn't it already in a f-string? What are you referring to?

Copy link
Member Author

Choose a reason for hiding this comment

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

No need for a variable is what I mean.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, now I get it. I'll fix it, I need some time. It's already nearing midnight and I'm doing homework. The Life of A Teenage Coder?

I will commit it, no problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you do that, I will not work on it though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can improve the layout if you make a better one window idea though.

Copy link
Member

Choose a reason for hiding this comment

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

But then how can we integrate the settings into our main window?

Copy link
Member Author

Choose a reason for hiding this comment

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

It already is. It could use a better design, sure, but a whole window for two settings? That's a little over the top.

Copy link
Member

Choose a reason for hiding this comment

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

We could repurpose the same window for settings too. Any ideas on this type? We could use a tabbed window. I'll explain.

@im-coder-lg im-coder-lg marked this pull request as ready for review August 20, 2023 17:38
@Moosems Moosems merged commit 98247f7 into main Aug 20, 2023
3 checks passed
@Moosems Moosems deleted the ui-updates branch August 20, 2023 19:09
@im-coder-lg im-coder-lg mentioned this pull request Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Dynamic Window Scaling
4 participants