Update PDF viewer to use toolbar layout and remove screenshot prevention#919
Update PDF viewer to use toolbar layout and remove screenshot prevention#919PruthiviRaj27 wants to merge 3 commits intomasterfrom
Conversation
|
@coderabbitai review |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PDF viewer activity was changed to extend a toolbar-enabled base class, removing the screenshot blocking method. The PDF viewer layout now includes a hidden toolbar view required by the new base class, while the PDF content remains fullscreen. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PdfViewerActivity
participant Toolbar
User->>PdfViewerActivity: onCreate()
PdfViewerActivity->>Toolbar: setVisibility(gone)
Note right of PdfViewerActivity: Toolbar included but hidden for BaseToolBarActivity
Estimated code review effort1 (~5 minutes) Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Actions performedReview triggered.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @PruthiviRaj27, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on refactoring the PdfViewerActivity to utilize a common base activity and integrate a shared toolbar component, which is then hidden. Additionally, it removes the functionality that previously prevented screen captures within the PDF viewer, streamlining the UI and adjusting security settings for the PDF viewing experience.
Highlights
- Base Class Refactoring: The
PdfViewerActivityhas been refactored to extendBaseToolBarActivityinstead ofAppCompatActivity, aligning it with a common base activity that likely provides standardized toolbar functionality. - Toolbar Integration and Hiding: A shared
testpress_toolbarlayout is now included inlayout_pdf_viewer.xmlforPdfViewerActivity. This toolbar is programmatically hidden immediately upon activity creation using a newhideToolBar()method. - Removal of Screen Capture Prevention: The
disableScreenShot()method, which previously usedWindowManager.LayoutParams.FLAG_SECUREto prevent screen captures, has been entirely removed fromPdfViewerActivity.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
coderabbitai
bot
changed the title
There was a problem hiding this comment.
Code Review
The pull request refactors PdfViewerActivity to inherit from BaseToolBarActivity and includes a toolbar in the layout. The toolbar is immediately hidden in onCreate. The removal of screenshot protection introduces a critical security vulnerability.
| disableScreenShot() | ||
| binding = LayoutPdfViewerBinding.inflate(layoutInflater) | ||
| setContentView(binding.root) | ||
| hideToolBar() |
There was a problem hiding this comment.
The toolbar is hidden immediately after the activity is created. Consider adding a comment explaining why the toolbar is hidden, or if it's never intended to be shown, consider using a more generic base activity instead of BaseToolBarActivity.
| <include android:id="@+id/tool_bar" | ||
| layout="@layout/testpress_toolbar" /> |
There was a problem hiding this comment.
A toolbar is included in this layout. Ensure that the toolbar is properly configured and styled to match the application's design guidelines.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
course/src/main/res/layout/layout_pdf_viewer.xml (1)
12-19: AnchorpdfViewbelow the toolbar to avoid overlap.
With the toolbar now pinned to the top,pdfViewshould start below it rather than at the very top of the parent.- app:layout_constraintTop_toTopOf="parent" + app:layout_constraintTop_toBottomOf="@id/tool_bar"This keeps the document fully visible and avoids rendering under an (even if hidden) toolbar.
🧹 Nitpick comments (1)
course/src/main/res/layout/layout_pdf_viewer.xml (1)
9-10: Toolbar addition looks good, but check PDFView constraints.The toolbar inclusion is properly implemented. However, note that the PDFView (lines 12-19) is still constrained to the parent's top (
app:layout_constraintTop_toTopOf="parent"), which means it will overlap with the toolbar if the toolbar is ever made visible.Consider updating the PDFView constraint to reference the toolbar:
- app:layout_constraintTop_toTopOf="parent" + app:layout_constraintTop_toBottomOf="@+id/tool_bar"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
course/src/main/java/in/testpress/course/ui/PdfViewerActivity.kt(3 hunks)course/src/main/res/layout/layout_pdf_viewer.xml(1 hunks)
🔇 Additional comments (3)
course/src/main/java/in/testpress/course/ui/PdfViewerActivity.kt (3)
12-13: Import additions look good.The new imports for
BaseToolBarActivityandisVisibleextension are correctly added to support the updated functionality.
15-15: Good consistency improvement with BaseToolBarActivity.Changing the inheritance from
AppCompatActivitytoBaseToolBarActivityimproves consistency across the application and provides better toolbar management capabilities.
15-15: No screenshot prevention was removed—FLAG_SECURE is still appliedThe
PdfViewerActivityinherits screenshot protection fromBaseToolBarActivity, which callswindow.setFlags( WindowManager.LayoutParams.FLAG_SECURE, WindowManager.LayoutParams.FLAG_SECURE )in its initialization. In
PdfViewerActivitythe onlywindow.setFlagscall is:this.window.setFlags( WindowManager.LayoutParams.FLAG_LAYOUT_NO_LIMITS, WindowManager.LayoutParams.FLAG_LAYOUT_NO_LIMITS )Since the mask (
FLAG_LAYOUT_NO_LIMITS) doesn’t clearFLAG_SECURE, PDF content remains protected from screenshots. No further changes are needed.Likely an incorrect or invalid review comment.
| disableScreenShot() | ||
| binding = LayoutPdfViewerBinding.inflate(layoutInflater) | ||
| setContentView(binding.root) | ||
| hideToolBar() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Question the design of adding a toolbar just to hide it.
The current implementation adds a toolbar to the layout and immediately hides it in onCreate(). This approach seems counterintuitive and adds unnecessary complexity.
Consider one of these alternatives:
- Remove the toolbar entirely if it's not needed for PDF viewing
- Show the toolbar conditionally based on user interaction or settings
- Use a different base activity that doesn't include toolbar functionality
- hideToolBar()
- hideStatusBar()
+ hideStatusBar()And remove the hideToolBar() method if the toolbar isn't needed.
Also applies to: 50-52
🤖 Prompt for AI Agents
In course/src/main/java/in/testpress/course/ui/PdfViewerActivity.kt around lines
31 and 50-52, the code adds a toolbar only to immediately hide it, which is
unnecessary. To fix this, remove the toolbar from the layout and any related
initialization code, including the hideToolBar() method, if the toolbar is not
needed for PDF viewing. Alternatively, implement logic to show the toolbar
conditionally based on user interaction or settings, or switch to a base
activity that does not include toolbar functionality.
| <include android:id="@+id/tool_bar" | ||
| layout="@layout/testpress_toolbar" /> |
There was a problem hiding this comment.
Toolbar include is missing mandatory constraints – build will fail with MissingConstraints.
Every direct child of ConstraintLayout must be constrained on at least one horizontal and one vertical axis. Lint will error and the view will default to (0,0) at runtime, possibly overlaying other content.
- <include android:id="@+id/tool_bar"
- layout="@layout/testpress_toolbar" />
+ <include
+ android:id="@+id/tool_bar"
+ layout="@layout/testpress_toolbar"
+ app:layout_constraintTop_toTopOf="parent"
+ app:layout_constraintStart_toStartOf="parent"
+ app:layout_constraintEnd_toEndOf="parent"/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <include android:id="@+id/tool_bar" | |
| layout="@layout/testpress_toolbar" /> | |
| <include | |
| android:id="@+id/tool_bar" | |
| layout="@layout/testpress_toolbar" | |
| app:layout_constraintTop_toTopOf="parent" | |
| app:layout_constraintStart_toStartOf="parent" | |
| app:layout_constraintEnd_toEndOf="parent"/> |
🤖 Prompt for AI Agents
In course/src/main/res/layout/layout_pdf_viewer.xml at lines 9 to 10, the
included toolbar view lacks the required constraints for ConstraintLayout,
causing build errors and layout issues. Add at least one horizontal and one
vertical constraint to the <include> element, such as
app:layout_constraintTop_toTopOf="parent" and
app:layout_constraintStart_toStartOf="parent", to properly position the toolbar
within the ConstraintLayout.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the PDF viewer to inherit from BaseToolBarActivity and removes screenshot prevention. The changes are clear and align with the PR's title.
My main feedback is on the architectural choice of extending BaseToolBarActivity for a fullscreen activity that doesn't display a toolbar. This requires adding a hidden toolbar to the layout, which could be a sign of a less-than-ideal base class hierarchy. I've left a comment suggesting a review of this choice to improve code clarity and maintainability.
| import `in`.testpress.ui.BaseToolBarActivity | ||
|
|
||
| class PdfViewerActivity : AppCompatActivity(), PdfDownloadListener, DisplayPDFListener { | ||
| class PdfViewerActivity : BaseToolBarActivity(), PdfDownloadListener, DisplayPDFListener { |
There was a problem hiding this comment.
Changing the base class to BaseToolBarActivity for a screen that is meant to be fullscreen and has no toolbar seems counter-intuitive. This change necessitates adding a gone toolbar to the layout file, which feels like a workaround and adds an unnecessary view.
Please consider if a more appropriate base class is available to extend, for example, one that provides common functionality without enforcing a toolbar.
If BaseToolBarActivity must be used for other essential logic it contains, it might indicate that the responsibilities in your base activities could be separated more cleanly (e.g., toolbar logic in one class, other common logic in another). This would improve long-term maintainability.
There was a problem hiding this comment.
@gemini-code-assist BaseToolBarActivity is the base activity for all other activities, providing common theme settings and privacy-related implementations.
For this specific activity, we don’t need the toolbar UI, but we still require the underlying privacy logic.
To avoid duplicating code, we continue to extend BaseToolBarActivity and simply hide the toolbar view.
There was a problem hiding this comment.
Thanks for the clarification, @PruthiviRaj27. I understand that BaseToolBarActivity serves as a common base for shared theme settings and privacy logic across your activities, and extending it here avoids code duplication for those functionalities.
While this approach works, it does introduce a slight architectural compromise by requiring a gone toolbar in the layout for an activity that doesn't visually use it. This can make the code less intuitive and potentially harder to maintain if BaseToolBarActivity accumulates more toolbar-specific logic in the future.
For long-term maintainability and clearer separation of concerns, consider refactoring your activity hierarchy. You could introduce a more fundamental BaseActivity (or similar) that handles only the common theme and privacy logic, and then have BaseToolBarActivity extend that class. This would allow PdfViewerActivity to extend the truly minimal base class, avoiding the need for a hidden toolbar and making the purpose of each base class more explicit.
1 participant
Summary by CodeRabbit
New Features
Chores