Freeseer / freeseer

Designed for capturing presentations at conferences. Pre-fill a list of talks to record, record them, and upload them to YouTube with our YouTube Uploader.
http://freeseer.readthedocs.org
GNU General Public License v3.0
215 stars 110 forks source link

Configtool Update Recording Tab #574

Closed farazs closed 9 years ago

farazs commented 9 years ago

fixes #540

recording tab screenshot

Note: There is a slight alignment issue caused by these modifications in the General tab because of the way ConfigTool is setup, everything in the different tabs is right-aligned unless it is forced to be left-aligned by something like a QLineEdit.

general tab screenshot

dideler commented 9 years ago

This is a great start! I'm going to review this in more depth tomorrow night by looking at the code changes.

Some quick feedback for this image so it matches the mockup:

recording

farazs commented 9 years ago

Alright so I made the section headers slightly larger and bold for both the Recording and General tab. Let me know if you feel this also resolves the second issue you brought up.

recording tab screenshot

Looking into the stretching now.

farazs commented 9 years ago

I've made the widgets in the recording tab a fixed size. I'm not sure if this is the right way to go about the stretching issue. It seems that because of how the ConfigToolWidget is set up, the right panel automatically follows the right edge of the config tool. This alignment issue might be more related to #541

zxiiro commented 9 years ago

As you may have discovered Qt resize rules are determined by a few Qt policies.

QtSizeHint: http://qt-project.org/doc/qt-4.8/qt.html#SizeHint-enum QtSizePolicy: http://qt-project.org/doc/qt-4.8/qsizepolicy.html

And QLayout (along with the other layout classes that inherit it): http://qt-project.org/doc/qt-4.8/qlayout.html

dideler commented 9 years ago

Please post new screenshots whenever you make noticeably significant changes to the UI. Saves everyone else a lot of time from having to run your code locally to view the UI.

farazs commented 9 years ago

Oh alright. Sorry about that.

Here it is: recording tab screenshot general tab screenshot

I ended up using NAME.capitalize() for the AboutWidget. It only capitalizes the first letter.

farazs commented 9 years ago

Thank you Thanh. I was mainly looking at SizePolicy, but I will look into SizeHint as well. It seems to be working well just with the SizePolicy changed to fixed and then changing the fixed size.

Are there any other changes that need to be made before this is good to go? Should I remove the changes regarding the AboutWidget minor refactoring and put those in a different pull request? Or should I just make that a separate commit when I squash the commits?

dideler commented 9 years ago

This is looking pretty good.

Alright so I made the section headers slightly larger and bold for both the Recording and General tab. Let me know if you feel this also resolves the second issue you brought up.

I noticed that I don't see the border for the groups on Ubuntu, I guess it's specific to Windows? So the alignment would still be a useful addition.

Are there any other changes that need to be made before this is good to go?

Alignment is broken for me in the Recording and General tabs (including the close button).

image

image

I'd like to see that fixed before merging this.

farazs commented 9 years ago

Here are the close button modifications : recording tab screenshot recording tab screenshot general tab screenshot general tab screenshot

dideler commented 9 years ago

Thanks. I saw in the IRC log that alignment with the section headers was difficult, so let's just drop it for now. Having it neatly aligned would make the UI more polished, but there's other stuff to do that can have more impact.

I'll take a look at the code changes now.

dideler commented 9 years ago

After you've responded to the feedback, you can squash the commits (you can use more than a single commit if you think some stuff doesn't belong in the same commit).

farazs commented 9 years ago

Okay I put the stylesheet into a variable called boxStyle. I put the dimensions into boxWidth and boxHeight. So they're not repeated multiple times.

I squashed the commits into one. Made it about Improving the ConfigTool as a whole so that some of the other minor changes can be put into it.

dideler commented 9 years ago

You're referencing the wrong issue in the commit message summary line. Should be #540, not #574 (which is the PR itself).

If the dimensions are constants (which I assume they are), the Python convention is to use ALL_CAPS for the name.

farazs commented 9 years ago

Ah okay thanks! That should be cleared up now.

dideler commented 9 years ago

Anyone else want to review before this gets merged?

farazs commented 9 years ago

Okay I switched the QLabel to a QSpacerItem, put a new line between the end of the else clause and the next statement, and only wrapped lines that went past 120 chars.

zxiiro commented 9 years ago

+1

Thanks

dideler commented 9 years ago

Merged as 6b5c1eabd90c430670cfbe63f98d827db4f4b396, thanks!